Last Comment Bug 684799 - Measure slop in more JS memory reporters
: Measure slop in more JS memory reporters
Status: RESOLVED FIXED
[MemShrink:P2][fixed-in-nanojit] fixe...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-09-05 23:25 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-11-17 17:29 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
JS patch (20.74 KB, patch)
2011-09-05 23:25 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
NJ patch (3.50 KB, patch)
2011-09-05 23:30 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
edwsmith: review-
Details | Diff | Review
JS patch, v2 (24.59 KB, patch)
2011-09-06 20:27 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
dmandelin: review+
Details | Diff | Review
NJ patch, v2 (1.45 KB, patch)
2011-09-06 20:31 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
edwsmith: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-05 23:25:30 PDT
Created attachment 558405 [details] [diff] [review]
JS patch

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).
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-05 23:30:22 PDT
Created attachment 558407 [details] [diff] [review]
NJ patch

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.
Comment 2 Edwin Smith 2011-09-06 06:27:34 PDT
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).
Comment 3 David Mandelin [:dmandelin] 2011-09-06 11:32:28 PDT
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?
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-06 12:58:14 PDT
> 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?
Comment 5 David Mandelin [:dmandelin] 2011-09-06 13:42:59 PDT
(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?
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-06 20:27:09 PDT
Created attachment 558720 [details] [diff] [review]
JS patch, v2

This patch uses a JSUsableSizeFun typedef, which I put in jsutil.h because I couldn't think of a better place for it.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-06 20:31:16 PDT
Created attachment 558721 [details] [diff] [review]
NJ patch, v2

> 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.
Comment 8 Edwin Smith 2011-09-07 08:46:32 PDT
(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.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-07 17:07:17 PDT
(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 Edwin Smith 2011-09-07 17:32:38 PDT
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.
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-09-07 18:18:37 PDT
http://hg.mozilla.org/projects/nanojit-central/rev/3fb37580d1ff

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