1364358 - Keep track of jemalloc thread-local arenas

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Description

[Mike Hommey [:glandium]]

Items left out in bug 1361258 that should be addressed in this bug by tracking the thread-local arenas, per the implementation commit message:
- jemalloc_stats doesn't know about them and will under-report memory
  usage.
- pre/post fork hooks don't know about them and will not force-unlock
  their locks. In practice, until those arenas are used for something
  else than the style system, this can't lead to the dead-locks that
  these hooks help prevent because nothing should be touching pointers
  allocated through them after fork.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1364358 - Remove mozjemalloc check for multithreading.

mozjemalloc had an optimization that shortcuts using mutexes when the
program is single-threaded. But with code evolution, the check whether
the program had multiple threads running was meant to be true all the
time.

In order to simplify the code, we just remove those checks and dead code
they were hiding in some cases.

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

As per explained 2 commits earlier, we remove the support for multiple
arenas. We however keep the `arenas` list and the `narenas` count to
use it them to track the opted-in per-thread arenas.

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

jemalloc_stats, as well as pre/post-fork hooks are using the `arenas`
list along the `narenas` count to iterate over all arenas setup by
mozjemalloc. Up until previous commit, that was used for automatic
multiple arenas support, which is now removed.

But mozjemalloc still supports running with multiple arenas, in the form
of opted-in, per-thread arenas. After bug 1361258, those arenas weren't
tracked, and now that `arenas` only contains the default arena, we can
now fill it with those thread-local arenas.

Keeping the automatic multiple arenas support, which we don't use and
don't really plan to, would have meant using a separate list for them.

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1364358 - Remove mozjemalloc arena balancing.

After bug 1361258, mozjemalloc uses a main arena for all allocations,
and specific threads can opt-in to use a different arena for that thread
only. Going forward, this is how we expect to support scaling across
different threads that require lots of concurrent allocations.

To simplify the mozjemalloc code, we'll remove the support for multiple
arenas.

This is the first step towards that, removing the support for arena
balancing. Everything behind the MOZ_BALANCE macro has never been used
in practice.

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

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8868045 [details]
Bug 1364358 - Remove mozjemalloc arena balancing.

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

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

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

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

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

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

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

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8868045 [details]
Bug 1364358 - Remove mozjemalloc arena balancing.

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

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

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

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

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

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

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

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8868045 [details]
Bug 1364358 - Remove mozjemalloc arena balancing.

https://reviewboard.mozilla.org/r/139636/#review143168

::: commit-message-1914a:12
(Diff revision 3)
> +
> +To simplify the mozjemalloc code, we'll remove the support for multiple
> +arenas.
> +
> +This is the first step towards that, removing the support for arena
> +balancing. Everything behind the MOZ_BALANCE macro has never been used

nit: 'MALLOC_BALANCE'

Comment 15

[Eric Rahm [:erahm]]

Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

https://reviewboard.mozilla.org/r/139630/#review143172

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

https://reviewboard.mozilla.org/r/139632/#review143176

::: memory/mozjemalloc/jemalloc.c
(Diff revision 3)
> -		malloc_spin_unlock(&arenas_lock);
> -	} else
>  		ret = arenas[0];
> -
> -#ifdef MOZ_MEMORY_WINDOWS
> +		RELEASE_ASSERT(ret != NULL);
> +	}
> -	TlsSetValue(tlsIndex, ret);

Since we're still doing the TLS lookup above, don't we want to continue setting the TLS arena?

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

https://reviewboard.mozilla.org/r/139634/#review143184

Looks good, just a few minor comments.

::: memory/mozjemalloc/jemalloc.c:4730
(Diff revision 4)
> +		abort();
> +
> +	return arenas[0];
> +}
> +
> +#define ARENAS_GROWTH 16

Can you add comment here? Considering the scope I'd almost rather have it be a const in the function instead (but still with a comment).

::: memory/mozjemalloc/jemalloc.c:4754
(Diff revision 4)
> +		size_t max_arenas = ((narenas + ARENAS_GROWTH) / ARENAS_GROWTH) * ARENAS_GROWTH;
> +		/*
> +		 * We're unfortunately leaking the previous allocation ;
> +		 * the base allocator doesn't know how to free things
> +		 */
> +		arena_t** new_arenas = (arena_t **)base_alloc(sizeof(arena_t *) * max_arenas);

I guess this is fine for now, I think I'd prefer a linked list where the head is statically allocated. Then for the `narenas[0]` cases we could avoid having to lock the arenas, but that could be a follow up or not at all.

::: memory/mozjemalloc/jemalloc.c:5496
(Diff revision 4)
>  	base_committed = 0;
>  #endif
>  	base_nodes = NULL;
>  	malloc_mutex_init(&base_mtx);
>  
> -	narenas = 1;
> +	malloc_spin_init(&arenas_lock);

Should we explicitly initialize `narenas = 0` here?

Comment 18

[Mike Hommey [:glandium]]

(In reply to Eric Rahm [:erahm] from comment #16)
> Comment on attachment 8868043 [details]
> Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.
> 
> https://reviewboard.mozilla.org/r/139632/#review143176
> 
> ::: memory/mozjemalloc/jemalloc.c
> (Diff revision 3)
> > -		malloc_spin_unlock(&arenas_lock);
> > -	} else
> >  		ret = arenas[0];
> > -
> > -#ifdef MOZ_MEMORY_WINDOWS
> > +		RELEASE_ASSERT(ret != NULL);
> > +	}
> > -	TlsSetValue(tlsIndex, ret);
> 
> Since we're still doing the TLS lookup above, don't we want to continue
> setting the TLS arena?

Actually, this is restored in the last patch, so meh?

Comment 19

[Mike Hommey [:glandium]]

(In reply to Eric Rahm [:erahm] from comment #17)
> ::: memory/mozjemalloc/jemalloc.c:5496
> (Diff revision 4)
> >  	base_committed = 0;
> >  #endif
> >  	base_nodes = NULL;
> >  	malloc_mutex_init(&base_mtx);
> >  
> > -	narenas = 1;
> > +	malloc_spin_init(&arenas_lock);
> 
> Should we explicitly initialize `narenas = 0` here?

statics are always initialized to 0.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Eric Rahm [:erahm] from comment #17)
> > +		arena_t** new_arenas = (arena_t **)base_alloc(sizeof(arena_t *) * max_arenas);
> 
> I guess this is fine for now, I think I'd prefer a linked list where the
> head is statically allocated. Then for the `narenas[0]` cases we could avoid
> having to lock the arenas, but that could be a follow up or not at all.

Will be more convenient when it's C++.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8868045 [details]
Bug 1364358 - Remove mozjemalloc arena balancing.

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

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

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

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

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

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

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

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8868045 [details]
Bug 1364358 - Remove mozjemalloc arena balancing.

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

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

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

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

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

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

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

Comment 29

[Eric Rahm [:erahm]]

Comment on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

https://reviewboard.mozilla.org/r/139632/#review143284

Comment 30

[Eric Rahm [:erahm]]

Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

https://reviewboard.mozilla.org/r/139634/#review143286

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83e7701f282e
Remove mozjemalloc arena balancing. r=erahm
https://hg.mozilla.org/integration/autoland/rev/0be410e68c53
Remove mozjemalloc check for multithreading. r=erahm
https://hg.mozilla.org/integration/autoland/rev/72a43cb4e932
Remove mozjemalloc support for automatic multiple arenas. r=erahm
https://hg.mozilla.org/integration/autoland/rev/bda4f18e7710
Keep track of mozjemalloc thread-local arenas. r=erahm

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83e7701f282e
https://hg.mozilla.org/mozilla-central/rev/0be410e68c53
https://hg.mozilla.org/mozilla-central/rev/72a43cb4e932
https://hg.mozilla.org/mozilla-central/rev/bda4f18e7710