Closed Bug 1402284 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Memory Allocator, enhancement, P1)

Product:
Core
Component:
Memory Allocator
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files)

Bug 1402284 - Make RedBlackTree::{Insert,Remove} work when the type has a constructor.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402284 - Initialize arena_t objects via a constructor instead of manually with an Init method.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402284 - Move arena tree related globals to a static singleton of a new class.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402284 - Separate arenas created from moz_arena_* functions from others.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
      No description provided.
Depends on: 1412722
Assignee: nobody → mh+mozilla
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().)
Attachment #8923618 - Flags: review?(n.nethercote) → review+
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.
Attachment #8923619 - Flags: review?(n.nethercote) → review+
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?
Attachment #8923620 - Flags: review?(n.nethercote)
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.
Attachment #8923621 - Flags: review?(n.nethercote) → review+
(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 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.
Attachment #8923620 - Flags: review?(n.nethercote) → review+
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 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
Pushed by mh@glandium.org:
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
Pushed by mh@glandium.org:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: