1364359 - Allow the removal of non-empty arenas, including thread local arenas.

Status: ASSIGNED

(Core :: Memory Allocator, enhancement, P3)

Description

[Mike Hommey [:glandium]]

The arenas created in the jemalloc_thread_local_arena function added in bug 1361258 are leaked on purpose. This bug is about not leaking them and properly terminating them (they might not be empty when jemalloc_thread_local_arena(false) is called).

Comment 1

[Sean Feng [:sefeng211]]

Can someone educate me the status of this bug?

I used moz_create_arena in bug 1377999, and I'd like know whether this is a blocker of my patch.

If I can't manually call moz_dispose_arena at the moment, will it be disposed sometime later?

Thanks

Comment 2

[Eric Rahm [:erahm]]

(In reply to Sean Feng [:sefeng] from comment #1)

Can someone educate me the status of this bug?

We think bug 1596300 will help out here, it's currently be worked on but is more of proof-of-concept right now.

I used moz_create_arena in bug 1377999, and I'd like know whether this is a blocker of my patch.

It really depends on how many you expect to create and whether or not you can reuse them. Mike probably has a better idea of the overhead of leaving them around.

If I can't manually call moz_dispose_arena at the moment, will it be disposed sometime later?

My understanding is they'll stick around until process shutdown. If these are limited to content processes this might not be a huge issue once fission is enabled.

Comment 3

[Mike Hommey [:glandium]]

What Eric said is accurate. At the moment, using moz_create_arena is not recommended except if the arena is meant to last until the end of the process. So for example, creating an arena for all DOM nodes in a process is fine, but creating an arena for all DOM nodes per page is not (with multiple pages per process).

Comment 4

[Jens Stutte [:jstutte]]

Note that currently we rely in quite some places on the arena not going away after having done gArenas.GetById outside the the collection's lock. We might be able to ensure that this pattern is used only on the main thread and add some API that allows us to run the effective dispose only ever on the main thread, too. Or in alternative we might want some additional mutex and/or reference counting for arenas.

Comment 5

[Paul Bone [:pbone]]

Bug 1950041 prevented the deletion of all arenas, but this bug still blocks public (thread-local) arena deletion. One potential solution would be either to merge arenas into an orphaned threads arena when a thread terminates, or to delete the arena once its last object is freed.

Comment 6

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - Allow destruction of thread-local arenas

Thread local arenas can now be destroyed, non-empty thread-local arenas will
be destroyed after their last object is freed, even if that's later from a
different thread. Thread local arenas will be destroyed implicitly when a
thread exits.

Comment 7

[Paul Bone [:pbone]]

It's strange but this blocks Bug 1980064 because thread-local structures in the allocator that refer to arenas may still be alive after an arena is destroyed. So we need to refcount arenas, and allow the removal of non-empty arenas, before we can implement TLS. That has the side-effect of no-longer leaking thread local arenas. So let's do that!

Comment 8

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 1. Mozjemalloc arenas are now refcounted

In the short term this will simplify logic that prevents an arena with a
purge request from being deleted before removing the purge request.

However the main motivation for this change is to keep an arena alive
until all thread local structures that refer to it have been cleaned up.

• Two new classes, RefPtrRedBlackTree and RefPtrDoublyLinkedList, have
  been created that wrap their namesakes and manipulates refcounts
  during insertion and removal.

• A RefPtrUtils.h file has been added that includes a safe refcounting
  mixin class.

• Because huge allocations now hold a refptr to the arena, deleting an
  arena that has only huge allocations will no-longer crash.
  TestJemalloc.cpp has been updated.

The places that hold references to arenas are:

• The ArenaCollection
• The purge request list.
• Huge allocations stored outside the arena structure but belonging to
  the arena.
• Some automatic stack variables when there's a chance another
  reference to the arena could be dropped, eg DisposeArena.

Thread local arenas are still leaked, threads have a raw pointer to
their arena, they are kept alive by ArenaCollection.

Comment 9

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 3. Simplify code for deleting arenas after purge

The delete statement was removed in the previous patch. This patch
removes some of the logic for deleting arenas after purge. It keeps
just enough logic so that purge can exit early for a dying arena.

Comment 10

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 4. Rename DisposeArena to RemoveArena

"remove" is more accurate since the arena might not be destroyed if
other things hold a reference.

Comment 11

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 5. Add locking annotation to mNumOperationsRemovedArenas

Comment 12

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 7. Support deleting non-empty arenas

Disposing a non-empty arena will now move it to a tree for disposed
arenas. This allows purging and jemalloc_stats to continue to process
it. Once it is empty it will be removed from this tree (dropping the
last reference).

Comment 13

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 9. Thread local arenas are no-longer leaked

Thread local arenas are now stored within a special class which removes
them from the arena collection when its destructor runs. Because arenas
aren't removed until they're empty this is safe and ensures that thread
local arenas won't be leaked.

Comment 14

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 10. Refactoring in arena_t::~arena_t()

Comment 15

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 1. Move the arena struct definition into a new header

Comment 16

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 8. Initialise the allocator in two phases

This makes it safe for thread local storage code supplied by the
libraries/compiler, such as C++'s thread_local keyword to call malloc.

malloc_initialised is now an enum to represent which phases have
completed.

malloc_init_hard() is called the first time that malloc() is used it
performs the first phase.

malloc_init_tls() is called the first time jemalloc_local_arena() is
used, it performs the second phase.

This also refactors choose_arena adding a branch annotation to the most
likely branch.

Comment 17

[Paul Bone [:pbone]]

Attachment: WIP: Bug 1364359 - pt 2. Clean up ArenaCollection iteration

The ArenaCollection iterator was always messy and caused at least one
bug in the past. A future patch will add weak references to arenas
which would make this much more complex. Replacing it with a simple
function that builds a vector may use more/different temporary storage,
but its conceptually much simpler.

Comment 18

[Phabricator Automation]

Comment on attachment 9533551 [details]
WIP: Bug 1364359 - pt 1. Move the arena struct definition into a new header

Revision D276789 was moved to bug 2016627. Setting attachment 9533551 [details] to obsolete.