Closed Bug 1364358 Opened 7 years ago Closed 7 years ago

Keep track of jemalloc thread-local arenas

Categories

(Core :: Memory Allocator, enhancement)

Product:
Core
Component:
Memory Allocator
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files)

Bug 1364358 - Remove mozjemalloc check for multithreading.
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1364358 - Remove mozjemalloc arena balancing.
59 bytes, text/x-review-board-request
erahm
: review+
Details 
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.
Blocks: 1365191
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'
Attachment #8868045 - Flags: review?(erahm) → review+
Comment on attachment 8868042 [details]
Bug 1364358 - Remove mozjemalloc check for multithreading.

https://reviewboard.mozilla.org/r/139630/#review143172
Attachment #8868042 - Flags: review?(erahm) → review+
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?
Attachment #8868043 - Flags: review?(erahm) → review-
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?
Attachment #8868044 - Flags: review?(erahm) → review-
(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?
(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.
(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 on attachment 8868043 [details]
Bug 1364358 - Remove mozjemalloc support for automatic multiple arenas.

https://reviewboard.mozilla.org/r/139632/#review143284
Attachment #8868043 - Flags: review?(erahm) → review+
Comment on attachment 8868044 [details]
Bug 1364358 - Keep track of mozjemalloc thread-local arenas.

https://reviewboard.mozilla.org/r/139634/#review143286
Attachment #8868044 - Flags: review?(erahm) → review+
Pushed by mh@glandium.org:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: