Measure slop in TI-related JS memory reporters

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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 :)
(Assignee)

Updated

6 years ago
Blocks: 563700
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 1

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

Comment 2

6 years ago
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.
Assignee: general → nnethercote
Status: NEW → ASSIGNED
Attachment #570889 - Flags: review?(bhackett1024)
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.
Attachment #570889 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 4

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

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1630a68499bc
https://hg.mozilla.org/mozilla-central/rev/1630a68499bc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.