685429 - Avoid wasted space in JSFunctionBoxQueue due to jemalloc rounding up

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

JSFunctionBoxQueue requests power-of-two buffers, but array_new() needs an extra slot to store the array size, which is enough to tip the request into jemalloc's next size class (e.g. 1024 -> 1032 -> 2048).  This one's my fault, because I introduced array_new() in bug 624878.

This accounts for something like 10% of all the cumulative slop bytes (10s of MBs) when opening Gmail and TechCrunch.  These allocations are probably short-lived and so don't affect the peak memory usage much, but fixing it will reduce malloc churn and so is worthwhile.

The attached patch does the easy fix, changing JSFunctionBoxQueue to use malloc_ instead of array_new.  (This works because JSFunctionBox doesn't have a constructor or destructor.)  It'd be better if array_new didn't have to store the value, but that's hard to do without changing how JS_USE_CUSTOM_ALLOCATOR works, which I'm not inclined to do right now.

Comment 1

[Paul Biggar]

Comment on attachment 559047 [details] [diff] [review]
patch

Review of attachment 559047 [details] [diff] [review]:
-----------------------------------------------------------------

Locally this is right, so r+ed, but it feels like we have a larger problem, perhaps to be solved in a different bug.

array_new is a trap for the unwary. Can we rename it to array_new_with_alignment_trap? Or make a version which doesn't call destructors and doesn't need the size stored? Perhaps array_new<WeDontCallDestructorsForYou> vs array_new<ThisAddsAnIntToYourSize>. I'm not sure, but I think it might be possible to type specialize on the presence/lack of destructors, how would that be?

Comment 2

[Paul Biggar]

Luke, see last paragraph in Comment 1.

Comment 3

[Luke Wagner [:luke]]

(In reply to Paul Biggar from comment #2)
Hmm, that is most unfortunate.  There isn't much difference between we-don't-call-destructors and just using malloc/free; basically the array versions save you a manual size computation.

Paul: with jemalloc is there any efficient way to snoop the size?

A last idea: what if we required the caller to pass the size to delete?  I suspect we usually have it on hand.  In debug, we could store the size to assert that the passed size was correct.

Comment 4

[Justin Lebar (not reading bugmail)]

> Paul: with jemalloc is there any efficient way to snoop the size?

You mean malloc_usable_size()?

Comment 5

[Paul Biggar]

(In reply to Justin Lebar [:jlebar] from comment #4)
> > Paul: with jemalloc is there any efficient way to snoop the size?
> 
> You mean malloc_usable_size()?

That tells us the sie of the piece of memory we've been passed, which can be larger than we asked for. It's also unreliable - it can return 0 in some cases, and I forget what it does if there's no jemalloc on that platform.

Comment 6

[Justin Lebar (not reading bugmail)]

> It's also unreliable - it can return 0 in some cases, and I forget what it does if there's no 
> jemalloc on that platform.

I think when you have jemalloc, it's not going to return 0?  But if there's no malloc_usable_size available through jemalloc or otherwise, then moz_malloc_usable_size will return 0.

Luke, if the question was, "Does jemalloc store N in malloc(N)?" then the answer is no.  Jemalloc only stores N rounded up to the next allocation size.

Comment 7

[Luke Wagner [:luke]]

Thanks for the info.  I am inclined toward the explicit-count-argument idea, then.

Comment 8

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bab147717076

Comment 9

[Nicholas Nethercote [inactive]]

I spun off bug 685754 for the array_new issue.

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bab147717076