Closed
Bug 684799
Opened 13 years ago
Closed 13 years ago
Measure slop in more JS memory reporters
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2][fixed-in-nanojit] fixed-in-tamarin)
Attachments
(2 files, 2 obsolete files)
24.59 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
This is much like 676732. This patch: - Uses moz_malloc_usable_size in all the JS memory reporters that don't already use it, except for the type inference ones, which I'll file a separate bug for. The list of ones it's newly used in: - tjit-data/allocators-main - tjit-data/allocators-reserve - tjit-data/trace-monitor - string-chars - property-tables - shape-kids (an entirely new reporter) - Removes HashMap::allocatedSize(), which duplicates the functionality of HashMap::sizeOf(). - Changes things so that the |mus| parameter in lots of functions is no longer optional, thereby simplifying them. You can pass an always-returns-zero function if you want to fall back to the non-malloc_usable_size size calculations (and shell/js.cpp does exactly this).
Attachment #558405 -
Flags: review?(dmandelin)
Assignee | ||
Comment 1•13 years ago
|
||
This patch: - Modifies Allocator to always use chunks of exactly 2KB, to avoid wasting space. - Uses a passed-in malloc_usable_size-style function in Allocator::getBytesAllocated() for maximum accuracy.
Attachment #558407 -
Flags: review?(edwsmith)
Comment 2•13 years ago
|
||
Comment on attachment 558407 [details] [diff] [review] NJ patch The change looks correct, so I feel guilty with R-. The "best" size for chunkbytes in tamarin is 2016, so allocating 2048 will waste about 2048 bytes. What do you think about modifying the allocChunk() interface function to return the pointer and the usable size, so it can internally round up the size to a "good" size? (a malloc implementation would call malloc_good_size(chunkbytes) before allocating or malloc_size(ptr) after allocation). This might also reduce waste when the nbytes paramter to fill is larger. A cheesier way would be to use a constant defined outside of nanojit.h that the vm controls -- TM would use 2048 and TR would use 2016 (instead of 2000, currently).
Attachment #558407 -
Flags: review?(edwsmith) → review-
Comment 3•13 years ago
|
||
Comment on attachment 558405 [details] [diff] [review] JS patch Review of attachment 558405 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks good, but using the parameter declaration |size_t(*mus)(void *)| everywhere seems kind of cryptic to me. How about adding a typedef for this and documenting the meaning at the point of the typedef?
Attachment #558405 -
Flags: review?(dmandelin)
Assignee | ||
Comment 4•13 years ago
|
||
> How about adding a typedef for
> this and documenting the meaning at the point of the typedef?
Sure. Any suggestions for the name of the typedef?
Assignee | ||
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 5•13 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4) > > How about adding a typedef for > > this and documenting the meaning at the point of the typedef? > > Sure. Any suggestions for the name of the typedef? How about JSMallocUsableSizeFun or JSUsableSizeFun?
Assignee | ||
Comment 6•13 years ago
|
||
This patch uses a JSUsableSizeFun typedef, which I put in jsutil.h because I couldn't think of a better place for it.
Attachment #558405 -
Attachment is obsolete: true
Attachment #558720 -
Flags: review?(dmandelin)
Assignee | ||
Comment 7•13 years ago
|
||
> The change looks correct, so I feel guilty with R-. Don't feel guilty! :) I'm trying to fix waste problems, not cause new ones. > The "best" size for > chunkbytes in tamarin is 2016, so allocating 2048 will waste about 2048 > bytes. Is that because TR adds some more tracking bytes to the front of Chunks? I took the easy option here and avoided changing how Chunks work. The bytes wasted for TM are minimal, I can live with them.
Attachment #558407 -
Attachment is obsolete: true
Attachment #558721 -
Flags: review?(edwsmith)
Comment 8•13 years ago
|
||
(In reply to Nicholas Nethercote [:njn] (on vacation Sep 12--Oct 17) from comment #7) > > The "best" size for > > chunkbytes in tamarin is 2016, so allocating 2048 will waste about 2048 > > bytes. > > Is that because TR adds some more tracking bytes to the front of Chunks? I traced through TR's allocChunk() down into the fixed-memory allocator, to find its ideal block size, and found: #ifdef MMGC_64BIT const static int kLargestAlloc = 2016; // The largest small-object allocation #else const static int kLargestAlloc = 2032; // The largest small-object allocation #endif Beyond those two sizes, allocation is done by rounding up to 4096 (page) size. the constants 2016 and 2032 reflect the way two allocations plus overhead fit into a 4096 byte page. (more for smaller size classes). So, translating, the "ideal" size should be ([2016|2032] - sizeof(Chunk)), for TR's allocator, and 2048-sizeof(Chunk) for jemalloc, since it has a 2048-byte size class. > > I took the easy option here and avoided changing how Chunks work. The bytes > wasted for TM are minimal, I can live with them. Cool. I've been following your blog (cool progress!) I'm in favor of trimming the fat as much as is practical.
Updated•13 years ago
|
Attachment #558720 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 9•13 years ago
|
||
(In reply to Edwin Smith from comment #8) > > > I took the easy option here and avoided changing how Chunks work. The bytes > > wasted for TM are minimal, I can live with them. > > Cool. I've been following your blog (cool progress!) I'm in favor of > trimming the fat as much as is practical. So are you happy with the minimal change patch?
Comment 10•13 years ago
|
||
Comment on attachment 558721 [details] [diff] [review] NJ patch, v2 Yes, fumble on my part, I meant to mark R+ along with the previous comment.
Attachment #558721 -
Flags: review?(edwsmith) → review+
Assignee | ||
Comment 11•13 years ago
|
||
http://hg.mozilla.org/projects/nanojit-central/rev/3fb37580d1ff
Whiteboard: [MemShrink:P2] → [MemShrink:P2][fixed-in-nanojit]
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/98ab80ff8992 http://hg.mozilla.org/integration/mozilla-inbound/rev/813746ec38bf
Whiteboard: [MemShrink:P2][fixed-in-nanojit] → [MemShrink:P2][fixed-in-nanojit][inbound]
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/98ab80ff8992 http://hg.mozilla.org/mozilla-central/rev/813746ec38bf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-nanojit][inbound] → [MemShrink:P2][fixed-in-nanojit]
Target Milestone: --- → mozilla9
Updated•13 years ago
|
Whiteboard: [MemShrink:P2][fixed-in-nanojit] → [MemShrink:P2][fixed-in-nanojit] fixed-in-tamarin
You need to log in
before you can comment on or make changes to this bug.
Description
•