Last Comment Bug 685429 - Avoid wasted space in JSFunctionBoxQueue due to jemalloc rounding up
: Avoid wasted space in JSFunctionBoxQueue due to jemalloc rounding up
Status: RESOLVED FIXED
[MemShrink][clownshoes]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-07 23:12 PDT by Nicholas Nethercote [:njn]
Modified: 2011-09-09 07:45 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (734 bytes, patch)
2011-09-07 23:12 PDT, Nicholas Nethercote [:njn]
paul.biggar: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-09-07 23:12:32 PDT
Created attachment 559047 [details] [diff] [review]
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 2011-09-08 00:07:18 PDT
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 2011-09-08 00:08:19 PDT
Luke, see last paragraph in Comment 1.
Comment 3 Luke Wagner [:luke] 2011-09-08 08:58:48 PDT
(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) 2011-09-08 09:47:47 PDT
> Paul: with jemalloc is there any efficient way to snoop the size?

You mean malloc_usable_size()?
Comment 5 Paul Biggar 2011-09-08 09:57:43 PDT
(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) 2011-09-08 10:02:30 PDT
> 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] 2011-09-08 11:10:37 PDT
Thanks for the info.  I am inclined toward the explicit-count-argument idea, then.
Comment 8 Nicholas Nethercote [:njn] 2011-09-08 17:32:49 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bab147717076
Comment 9 Nicholas Nethercote [:njn] 2011-09-08 17:56:01 PDT
I spun off bug 685754 for the array_new issue.

Note You need to log in before you can comment on or make changes to this bug.