1402284 - Make it impossible to use arenas not created with moz_create_arena with moz_arena_* functions

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement, P1)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1402284 - Make RedBlackTree::{Insert,Remove} work when the type has a constructor.

RedBlackTree::{Insert,Remove} allocate an object on the stack for its
RedBlackTreeNode, and that shouldn't have side effects if the type
happens to have a constructor. This will allow to add constructors to
some of the mozjemalloc types.

Review commit: https://reviewboard.mozilla.org/r/194750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194750/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

Review commit: https://reviewboard.mozilla.org/r/194752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194752/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

We create the arena_t::Collection class to handle operations on the
arena tree. Ideally, iter() would trigger locking, but but the
prefork/postfork code complicates things, so we leave this for later.

Review commit: https://reviewboard.mozilla.org/r/194754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194754/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

We introduce the notion of private arenas, separate from other arenas
(main and thread-local). They are kept in a separate arena tree, and
arena lookups from moz_arena_* functions only access the tree of
private arenas. Iteration still goes through all arenas, private and
non-private.

Review commit: https://reviewboard.mozilla.org/r/194756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194756/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8923619 [details]
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194752/diff/1-2/

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194754/diff/1-2/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194756/diff/1-2/

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8923618 [details]
Bug 1402284 - Make RedBlackTree::{Insert,Remove} work when the type has a constructor.

https://reviewboard.mozilla.org/r/194750/#review200236

::: memory/build/rb.h:338
(Diff revision 1)
>      return ret;
>    }
>  
>    void Insert(TreeNode* aNode)
>    {
> -    TreeNode rbp_i_s;
> +    mozilla::AlignedStorage2<TreeNode> rbp_i_s;

Please add a comment explaining the use of AlignedStorage2 -- to avoid constructors on the element type.

::: memory/build/rb.h:422
(Diff revision 1)
>      mRoot->SetColor(NodeColor::Black);
>    }
>  
>    void Remove(TreeNode* aNode)
>    {
> -    TreeNode rbp_r_s;
> +    mozilla::AlignedStorage2<TreeNode> rbp_r_s;

Ditto. (This comment could refer to the one in Insert().)

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8923619 [details]
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

https://reviewboard.mozilla.org/r/194752/#review200238

::: memory/build/mozjemalloc.cpp:979
(Diff revision 2)
>    //       35  | 1024 |
>    //       36  | 2048 |
>    //   --------+------+
>    arena_bin_t mBins[1]; // Dynamically sized.
>  
> -  bool Init();
> +  arena_t();

Seeing this makes me think it would be nice if various types were eventually renamed as Arena, ArenaChunk, etc.

::: memory/build/mozjemalloc.cpp:1056
(Diff revision 2)
> +                      (sizeof(arena_bin_t) * (ntbins + nqbins + nsbins - 1)));
> +  }
> +
> +  void operator delete(void*)
> +  {
> +    // Base allocator can't free. Leak.

Is this function ever called? Should it MOZ_CRASH, or be `delete`d?

::: memory/build/mozjemalloc.cpp:4779
(Diff revision 2)
>  arena_t::GetById(arena_id_t aArenaId)
>  {
>    if (!malloc_initialized) {
>      return nullptr;
>    }
> -  arena_t key;
> +  mozilla::AlignedStorage2<arena_t> key;

Again, please add a comment about the use of AlignedStorage2.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

https://reviewboard.mozilla.org/r/194754/#review200240

I have some issues with the names used in this patch. I have some suggestions below that I'm not entirely happy with. Maybe you can think of better names?

::: commit-message-37d4b:4
(Diff revision 2)
> +Bug 1402284 - Move arena tree related globals to a static singleton of a new class. r?njn
> +
> +We create the arena_t::Collection class to handle operations on the
> +arena tree. Ideally, iter() would trigger locking, but but the

"but but"

::: memory/build/mozjemalloc.cpp:1072
(Diff revision 2)
> -    MOZ_ASSERT(aOther);
> +      MOZ_ASSERT(aOther);
> -    return (aNode->mId > aOther->mId) - (aNode->mId < aOther->mId);
> +      return (aNode->mId > aOther->mId) - (aNode->mId < aOther->mId);
> -  }
> +    }
> -};
> +  };
>  
> +  class Collection

I find `Collection` a non-descriptive name. Maybe `State` or `GlobalData`? Not sure.

I also wonder if moving this class outside of arena_t would help things. E.g. instead of having `arena_t::sCollection` everywhere you could just have `gArenaState` or `gArenas` or something like that.

::: memory/build/mozjemalloc.cpp:1084
(Diff revision 2)
> +      return bool(mMainArena);
> +    }
> +
> +    inline arena_t* GetById(arena_id_t aArenaId);
> +
> +    arena_t* Create();

Having methods called Init() and Create() is a bit confusing. Would this be better called CreateArena()? Or maybe CreateAndAdd(), to indicate that the arena is created and also added to the collection?

::: memory/build/mozjemalloc.cpp:1086
(Diff revision 2)
> +
> +    inline arena_t* GetById(arena_id_t aArenaId);
> +
> +    arena_t* Create();
> +
> +    void Dispose(arena_t* aArena)

And this could be DisposeArena() or DisposeAndRemoveArena()?

::: memory/build/mozjemalloc.cpp:3887
(Diff revision 2)
>  }
>  
> -static inline arena_t*
> -arenas_fallback()
> +arena_t*
> +arena_t::Collection::Create()
>  {
> +  arena_t* ret = new arena_t();

It's unfortunate that this `operator new` is fallible, as opposed to the normal infallible `new` in Gecko...

::: memory/build/mozjemalloc.cpp
(Diff revision 2)
> -  if (!gMainArena) {
>      return false;
>    }
> -  // arena_t constructor sets this to a lower value for thread local arenas;
> -  // reset to the default value for the main arena.
> -  gMainArena->mMaxDirty = opt_dirty_max;

mMaxDirty isn't set anywhere in the new code. Should it be?

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

https://reviewboard.mozilla.org/r/194756/#review200250

::: memory/build/mozjemalloc.cpp:1083
(Diff revision 2)
> -      mMainArena = mLock.Init() ? Create() : nullptr;
> +      mPrivateArenas.Init();
> +      mMainArena = mLock.Init() ? Create(/* private = */ false) : nullptr;
>        return bool(mMainArena);
>      }
>  
> -    inline arena_t* GetById(arena_id_t aArenaId);
> +    inline arena_t* GetById(arena_id_t aArenaId, bool aPrivate = true);

aIsPrivate is better than aPrivate (throughout the whole patch).

I also think it would be better if this param didn't have a default.

::: memory/build/mozjemalloc.cpp:1140
(Diff revision 2)
>  
>    private:
>      arena_t* mMainArena;
>      arena_id_t mLastArenaId;
> -    RedBlackTree<arena_t, ArenaTreeTrait> mArenas;
> +    Tree mArenas;
> +    Tree mPrivateArenas;

I don't find "private" very descriptive, though I'm having trouble coming up with an alternative name that clearly indicates "not main and not thread-local". At the least, there should be a comment here explaining what "private" means.

It also makes me wonder if it would be better to separate the main arena from the thread-local arenas. I.e. have mMainArena (not just a pointer), mThreadLocalArenas, mPrivateArenas. Though that may just complicate things, I'm not sure.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > +  void operator delete(void*)
> > +  {
> > +    // Base allocator can't free. Leak.
> 
> Is this function ever called? Should it MOZ_CRASH, or be `delete`d?

Right now, it's not, so I guess it could be `delete`d for now. Although eventually it will have a real implementation.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8923618 [details]
Bug 1402284 - Make RedBlackTree::{Insert,Remove} work when the type has a constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194750/diff/1-2/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8923619 [details]
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194752/diff/2-3/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194754/diff/2-3/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194756/diff/2-3/

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

https://reviewboard.mozilla.org/r/194754/#review200304

::: commit-message-37d4b:3
(Diff revision 3)
> +Bug 1402284 - Move arena tree related globals to a static singleton of a new class. r?njn
> +
> +We create the arena_t::Collection class to handle operations on the

This needs updating.

::: memory/build/mozjemalloc.cpp:1078
(Diff revision 3)
> +
> +  using Iterator = RedBlackTree<arena_t, ArenaTreeTrait>::Iterator;
> +
> +  Iterator iter() { return mArenas.iter(); }
> +
> +  inline arena_t* GetMain() { return mMainArena; }

Just call it Main()?

::: memory/build/mozjemalloc.cpp
(Diff revision 3)
> -  if (!gMainArena) {
>      return false;
>    }
> -  // arena_t constructor sets this to a lower value for thread local arenas;
> -  // reset to the default value for the main arena.
> -  gMainArena->mMaxDirty = opt_dirty_max;

Still not sure about this.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

https://reviewboard.mozilla.org/r/194756/#review200308

::: memory/build/mozjemalloc.cpp:1124
(Diff revision 3)
>  
>  private:
> -  arena_t* mMainArena;
> +  arena_t* mDefaultArena;
>    arena_id_t mLastArenaId;
> -  RedBlackTree<arena_t, ArenaTreeTrait> mArenas;
> +  Tree mArenas;
> +  Tree mPrivateArenas;

Still no comment here about the distinction between mArenas and mPrivateArenas.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

https://reviewboard.mozilla.org/r/194756/#review200308

> Still no comment here about the distinction between mArenas and mPrivateArenas.

See above the class definition

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8923618 [details]
Bug 1402284 - Make RedBlackTree::{Insert,Remove} work when the type has a constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194750/diff/2-3/

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8923619 [details]
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194752/diff/3-4/

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194754/diff/3-4/

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194756/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b7818155442
Make RedBlackTree::{Insert,Remove} work when the type has a constructor. r=njn
https://hg.mozilla.org/integration/autoland/rev/8f83dc111aed
Initialize arena_t objects via a constructor instead of manually with an Init method. r=njn
https://hg.mozilla.org/integration/autoland/rev/a0c193d65799
Move arena tree related globals to a static singleton of a new class. r=njn
https://hg.mozilla.org/integration/autoland/rev/f894ea204bb4
Separate arenas created from moz_arena_* functions from others. r=njn

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/e626359954d0 for https://treeherder.mozilla.org/logviewer.html#?job_id=141226870&repo=autoland

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8923619 [details]
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194752/diff/4-5/

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8923620 [details]
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194754/diff/4-5/

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8923621 [details]
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194756/diff/4-5/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8cd04b9941e
Make RedBlackTree::{Insert,Remove} work when the type has a constructor. r=njn
https://hg.mozilla.org/integration/autoland/rev/599a8c1526bc
Initialize arena_t objects via a constructor instead of manually with an Init method. r=njn
https://hg.mozilla.org/integration/autoland/rev/23acf21b5642
Move arena tree related globals to a static singleton of a new class. r=njn
https://hg.mozilla.org/integration/autoland/rev/8972c662b24f
Separate arenas created from moz_arena_* functions from others. r=njn

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d8cd04b9941e
https://hg.mozilla.org/mozilla-central/rev/599a8c1526bc
https://hg.mozilla.org/mozilla-central/rev/23acf21b5642
https://hg.mozilla.org/mozilla-central/rev/8972c662b24f