560818 - Use jemalloc chunk allocation for JS GC.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Igor Bukanov]

With bug 553812 fixed it is possible to use jemalloc chunk allocation code to get JS GC chunks. This allows to extend the memory statistics and usage available in jemalloc to cover the GC chunks. To implement this it is necessary to expose the chunk allocator from jemalloc so external callers can use it. Another complication is that on Linux SpiderMonkey is not linked against jemalloc and instead relies on weak symbols to substitute GLIBC malloc with jemalloc at load time (correct me if I get this wrong). So as a workaround I will use the proposed API from the bug 557538 to substitute the chunk allocation at the run time.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch juts exposes jemalloc chunk allocator using separated function, there are no callers of those.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: use jemalloc if available, add memory reporter

(Stealing this back from igor)

With Igor's work to add custom gc chunk allocator support, we can then do this in xpconnect, like so.  This has us use jemalloc if it's enabled, and also adds gc chunk memory tracking in either case.

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 449978 [details] [diff] [review]
use jemalloc if available, add memory reporter

Yeah, looks good. r=shaver

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/tracemonkey/rev/99ce939d4545

Now just need to get it to trunk!

Comment 5

[Igor Bukanov]

Comment on attachment 449978 [details] [diff] [review]
use jemalloc if available, add memory reporter

>+#ifdef MOZ_MEMORY
>+        posix_memalign(&chunk, js::GC_CHUNK_SIZE, js::GC_CHUNK_SIZE);
>+#else
>+        chunk = js::AllocGCChunk();
>+#endif

This assumes that js::GC_CHUNK_SIZE <= chunkSize from jemalloc not to waste twice the memory. Yet chunkSize can be altered via an environment variable or a configuration file so it would be nice to warn about inefficiencies. But perhaps we should just make chunkSize a static constant when jemalloc is built with MOZ_MEMORY defined. It may even speedup things little bit.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, chunkSize meaning the jemalloc allocation size?  That's a good point -- right now it looks like jemalloc's default chunk size is 1MB, which is the same as what JS wants, so there shouldn't be any wasted space.  But you're right, if the jemalloc chunk size goes up we'll end up allocating that amount, and if the JS chunk size goes up to more than jemalloc's, we'll end up allocating a bunch of extra as well.

Making it static and asserting that they match would probably be a good solution, though I'm not sure where the assertion should live.. the chunk size is an internal jemalloc thing.  Maybe something like:

#ifdef DEBUG
    NS_ASSERTION(malloc_usable_size(chunk) == js::GC_CHUNK_SIZE, "jemalloc allocated more than needed for JS, ensure jemalloc and JS chunk sizes are in sync");
#endif

Another option is to just not allocate the memory from jemalloc at all, and just track it through these allocators.  Would be better if everything went through jemalloc though.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> #ifdef DEBUG
>     NS_ASSERTION(malloc_usable_size(chunk) == js::GC_CHUNK_SIZE, "jemalloc
> allocated more than needed for JS, ensure jemalloc and JS chunk sizes are in
> sync");
> #endif

Yes, something like that would be nice.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> (In reply to comment #6)
> > #ifdef DEBUG
> >     NS_ASSERTION(malloc_usable_size(chunk) == js::GC_CHUNK_SIZE, "jemalloc
> > allocated more than needed for JS, ensure jemalloc and JS chunk sizes are in
> > sync");
> > #endif
> 
> Yes, something like that would be nice.

On the other hand the chunksize in jemalloc can be changed at runtime via configuration file. So the assert as a protection against chunk size mismatch is not very meaningful. I think we should really change jemalloc to use a compile-time constant.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/99ce939d4545