899256 - Rework JS memory reporters so gc-heap is no longer a top-level node.

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

+++ This bug was initially created as a clone of Bug #893222 +++

In bug 893222 we're reporting more individual strings in about:memory.

Right now we report these strings as

  gc-heap/notable/string(...)
  string-chars/notable/string(...)

This bug is about getting rid of gc-heap as a top-level node and rearranging the memory reporters so we can have


  strings
    gc-things
    string-chars

While we're at it, we're going to change some other reporters to match this new idiom.

Comment 1

[Justin Lebar (not reading bugmail)]

For example:

> 0.57 MB (00.70%) -- compartment([System Principal], file:///Users/jlebar/code/moz/ff-git/debug/dist/NightlyDebug.app/Contents/MacOS/components/nsSearchService.js)
> ├──0.19 MB (00.24%) -- objects
> │  ├──0.15 MB (00.19%) -- gc-heap
> │  │  ├──0.09 MB (00.11%) ── function
> │  │  ├──0.05 MB (00.07%) ── ordinary
> │  │  └──0.01 MB (00.01%) ── dense-array
> │  └──0.04 MB (00.05%) -- malloc-heap
> │     ├──0.02 MB (00.03%) ── slots
> │     └──0.02 MB (00.02%) ── elements/non-asm.js
> ├──0.18 MB (00.22%) -- shapes
> │  ├──0.12 MB (00.15%) -- gc-heap
> │  │  ├──0.06 MB (00.08%) -- tree
> │  │  │  ├──0.05 MB (00.07%) ── global-parented
> │  │  │  └──0.01 MB (00.01%) ── non-global-parented
> │  │  ├──0.05 MB (00.06%) ── base
> │  │  └──0.01 MB (00.01%) ── dict
> │  └──0.06 MB (00.07%) -- malloc-heap
> │     ├──0.03 MB (00.04%) ── compartment-tables
> │     ├──0.02 MB (00.02%) ── tree-tables
> │     └──0.01 MB (00.01%) ── tree-shape-kids
> ├──0.09 MB (00.12%) ── type-inference/analysis-pool
> ├──0.05 MB (00.06%) -- scripts
> │  ├──0.04 MB (00.05%) ── gc-things
> │  └──0.01 MB (00.01%) ── malloc-heap/data
> ├──0.03 MB (00.03%) -- baseline/stubs
> │  ├──0.02 MB (00.02%) ── fallback
> │  └──0.01 MB (00.01%) ── optimized
> ├──0.02 MB (00.02%) -- sundries
> │  ├──0.01 MB (00.02%) ── malloc-heap
> │  └──0.00 MB (00.01%) ── gc-heap
> └──0.01 MB (00.01%) ── cross-compartment-wrapper-table

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Rework JS memory reporters so gc-heap is no longer a top-level node.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1.1

Forgot to qref.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 782739 [details] [diff] [review]
Patch, v1.1

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

For GC things that are broken into additional categories, you have "gc-heap", e.g.:

> > ├──0.19 MB (00.24%) -- objects
> > │  ├──0.15 MB (00.19%) -- gc-heap
> > │  │  ├──0.09 MB (00.11%) ── function
> > │  │  ├──0.05 MB (00.07%) ── ordinary
> > │  │  └──0.01 MB (00.01%) ── dense-array
> > │  └──0.04 MB (00.05%) -- malloc-heap
> > │     ├──0.02 MB (00.03%) ── slots
> > │     └──0.02 MB (00.02%) ── elements/non-asm.js

For other GC things, you have "gc-things", e.g.:

> > ├──0.05 MB (00.06%) -- scripts
> > │  ├──0.04 MB (00.05%) ── gc-things
> > │  └──0.01 MB (00.01%) ── malloc-heap/data

It would be better to use a single term consistently.  I prefer "gc-heap", because it (a) matches "malloc-heap", and (b) matches the various other "gc-heap" subtrees we have for non-GC-things, e.g. the one containing "chunk-admin", "unused-arenas" and "unused-chunks".

That will leave the following paths containing "gc-things", but they're all within "gc-heap" sub-trees, so I think that's reasonable:
- *zone*/gc-heap/unused-gc-things
- js-main-runtime-gc-heap-committed/used/gc-things
- js-main-runtime-gc-heap-committed/unused/gc-things

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1421,5 @@
>  {
>      const nsAutoCString& pathPrefix = extras.pathPrefix;
>      size_t gcTotal = 0, gcHeapSundries = 0, otherSundries = 0;
>  
> +    ZCREPORT_GC_BYTES(pathPrefix + NS_LITERAL_CSTRING("gc-arena-admin"),

"gc-heap-arena-admin", for additional consistency?

@@ +1496,5 @@
> +            "under 'string-chars/.",
> +            JS::NotableStringInfo::notableSize()));
> +        gcTotal += info.totalGCThingSizeOf();
> +
> +        if (info.sizeOfAllStringChars) {

I prefer |info.sizeOfAllStringChars > 0| for integer non-zero tests.

@@ +1537,2 @@
>                          gcHeapSundries,
> +                        "The sum of all the 'gc-heap' and 'gc-things' measurements "

"and 'gc-things'" can be removed.

@@ +1794,2 @@
>                          gcHeapSundries,
> +                        "The sum of all the gc-heap and gc-things "

Single quotes around 'gc-heap'.  "and gc-things" can be removed.

@@ +1803,2 @@
>                       nsIMemoryReporter::KIND_HEAP, otherSundries,
> +                     "The sum of all the malloc-heap "

Single quotes around 'malloc-heap'.

Comment 5

[Justin Lebar (not reading bugmail)]

> It would be better to use a single term consistently.

My reasoning for gc-heap and gc-things is in bug 893222 comment 19: It seemed strange to me to use gc-heap both as an "adjective" (non-leaf node) and a noun (leaf node).  But I don't feel strongly about this.

Comment 6

[Justin Lebar (not reading bugmail)]

If we're doing gc-heap as a leaf node, should I change the strings reporters from 'malloc-heap/string-chars' to 'malloc-heap'?  That seems consistent to me.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

Comment 8

[Justin Lebar (not reading bugmail)]

The git branch is updated with the latest patches if you want to pull.  I haven't rebased the branch, so whatever merge conflicts you hit last time you applied the patches you'll probably hit again, if you try to apply the patches.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 783676 [details] [diff] [review]
Patch, v2

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

LGTM.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1468,5 @@
>      size_t gcHeapStringsAboutMemory = 0;
>      size_t stringCharsAboutMemory = 0;
>  
>      for (size_t i = 0; i < zStats.notableStrings.length(); i++) {
> +        const JS::NotableStringInfo& info = zStats.notableStrings[i];

Ah... xpconnect style is a total disaster :/

Comment 10

[Justin Lebar (not reading bugmail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0bf28abdf2

Comment 11

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9806317bc14

Comment 12

[Justin Lebar (not reading bugmail)]

^ That was a bustage backout.

Comment 13

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb6f1e0cdf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/019c0b931ad4

The second cset there is just moving more "nsIMemoryReporter::" into macros.  Somehow one instance of REPORT(..., nsIMemoryReporter::FOO) I accidentally left in broke only some builds, so I figured it would be good to get rid of all the "nsIMemoryReporter::"'s outside of the macros.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bb6f1e0cdf9
https://hg.mozilla.org/mozilla-central/rev/019c0b931ad4