684800 - Measure slop in TI-related JS memory reporters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

Note to self: remove allocatedSize() from HashTable, HashSet et al once this is done -- sizeOf() replaces it.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: 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 [Laid off!]]

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 [inactive]]

> 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 [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1630a68499bc

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1630a68499bc