Last Comment Bug 684800 - Measure slop in TI-related JS memory reporters
: Measure slop in TI-related JS memory reporters
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-09-05 23:30 PDT by Nicholas Nethercote [:njn]
Modified: 2011-11-02 06:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.16 KB, patch)
2011-10-31 17:24 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-09-05 23:30:49 PDT
This is much like bug 684799 (see the patch in that bug), but for the JS memory reporters relating to TI.

bhackett, I was going to do them as part of bug 684799 but I found the change non-trivial enough that I decided to do it as a separate bug so as not to slow down the changes to the other reporters.  (The |stats->scripts += bytes;
stats->temporary -= bytes;| lines in GetScriptMemoryStats() particularly bamboozled me.)

I'm going on vacation for a month at the end of the week and I'll get back to this after that, but if you want to do this bug in the meantime that'd be fine by me :)
Comment 1 Nicholas Nethercote [:njn] 2011-10-27 19:33:33 PDT
Note to self: remove allocatedSize() from HashTable, HashSet et al once this is done -- sizeOf() replaces it.
Comment 2 Nicholas Nethercote [:njn] 2011-10-31 17:24:41 PDT
Created attachment 570889 [details] [diff] [review]
patch

This patch changes the TI memory reporters to use moz_malloc_usable_size.  
Notable changes:

- TypeObjects were being counted twice, once in 
  XPCJSRuntime.cpp:CellCallback(), and once in 
  JS_GetTypeInferenceObjectStats().  This patch removes the latter case.o
  
- Hash{Table,Map,Set}::allocatedSize() is no longer needed, sizeOf() is 
  better.
  
- The total space used by the temp pool is now measured, including headers 
  and unused space.  This matches how other reporters work -- we don't want
  to ignore any memory if we can avoid it.  (We could split the reporter 
  into used/unused if we wanted, but I don't think that's much use here.)
  
- The TypeScriptNesting for each script is now counted.  I had to make it 
  public, which isn't great.
Comment 3 Brian Hackett (:bhackett) 2011-11-01 08:30:05 PDT
Comment on attachment 570889 [details] [diff] [review]
patch

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

::: js/src/jsinfer.cpp
@@ +6197,5 @@
>      /* Pending arrays are cleared on GC along with the analysis pool. */
> +    size_t usable = usf(compartment->types.pendingArray);
> +    stats->temporary +=
> +        usable ? usable
> +               : sizeof(TypeCompartment::PendingWork) * compartment->types.pendingCapacity;

Separate issue, but to reiterate from our discussion yesterday:

It would be nice if the UsableSizeFun signature took a void* AND the amount of space used, which would avoid duplicating this 'usable ? usable : ...' logic everywhere the function is invoked.  Even if the amount of slop isn't exposed through about:memory, it also allows cross checking info and making sure the amount of allocated space is >= the amount of used space.
Comment 4 Nicholas Nethercote [:njn] 2011-11-01 18:36:32 PDT
> It would be nice if the UsableSizeFun signature took a void* AND the amount
> of space used, which would avoid duplicating this 'usable ? usable : ...'
> logic everywhere the function is invoked.  Even if the amount of slop isn't
> exposed through about:memory, it also allows cross checking info and making
> sure the amount of allocated space is >= the amount of used space.

I filed bug 698968 for this.
Comment 5 Nicholas Nethercote [:njn] 2011-11-01 22:04:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1630a68499bc
Comment 6 Ed Morley [:emorley] 2011-11-02 06:31:00 PDT
https://hg.mozilla.org/mozilla-central/rev/1630a68499bc

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