The default bug view has changed. See this FAQ.

Avoid wasted space in JSFunctionBoxQueue due to jemalloc rounding up

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla9
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink][clownshoes])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
Attachment #559047 - Flags: review?(pbiggar)

Comment 1

6 years ago
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?
Attachment #559047 - Flags: review?(pbiggar) → review+

Comment 2

6 years ago
Luke, see last paragraph in Comment 1.

Comment 3

6 years ago
(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.
> Paul: with jemalloc is there any efficient way to snoop the size?

You mean malloc_usable_size()?

Comment 5

6 years ago
(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.
> 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

6 years ago
Thanks for the info.  I am inclined toward the explicit-count-argument idea, then.
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bab147717076
Whiteboard: [MemShrink][clownshoes] → [MemShrink][clownshoes][inbound]
(Assignee)

Comment 9

6 years ago
I spun off bug 685754 for the array_new issue.
http://hg.mozilla.org/mozilla-central/rev/bab147717076
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][clownshoes][inbound] → [MemShrink][clownshoes]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.