755583 - Streamline JS compartment reporters

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

Compartment-per-global means we have many more compartments than before.  And there's scope for distinguishing many of the ones that are currently merged in about:memory (e.g. see bug 754771).

But the generation of about:memory itself consumes memory, and it consumes more as the amount of text in about:memory increases.  In other words, about:memory perturbs what it is measuring.

More information is a good thing, so I'd like to reduce this perturbation, either by making about:memory's generation more efficient, or reducing the number of things reported per compartment, or both.

But to really make a difference, we'll require better measurement of the perturbation, something we currently lack.

Comment 1

[Nicholas Nethercote [inactive]]

> But to really make a difference, we'll require better measurement of the
> perturbation, something we currently lack.

Bug 720595 does this.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Patch 1: introduced "sundries" memory reports for compartments

We have 30+ measurements per compartment.  With CPG landing, and bug 754771
promising to distinguish the many System Principle compartments (i.e. so
they won't be aggregated in about:memory), about:memory is bloated with lots
of small, uninteresting measurements.  This also perturbs what's being
measured.

This patch modifies the js multi-reporters to merge all measurements for a
single compartment that are below 4KB into a "sundries" entry.  Actually,
there is a "gc-heap/sundries" entry (which is KIND_NONHEAP) and a
"other-sundries" (which is KIND_HEAP);  the two kinds must be kept separate
otherwise bad things happen.

With patch two from bug 754771 applied, this reduces the size of the
verbose "explicit" tree at start-up from ~3000 lines to ~1500 lines, and
roughly halves memory consumed during generation of about:memory.

Here's an example of a very small compartment after this change:

│  ├──────22,304 B (00.03%) -- compartment([System Principal], chrome://global/content/bindings/splitter.xml)
│  │      ├──20,480 B (00.03%) -- gc-heap
│  │      │  ├──19,688 B (00.03%) -- arena
│  │      │  │  └──19,688 B (00.03%) ── unused
│  │      │  └─────792 B (00.00%) ── sundries
│  │      └───1,824 B (00.00%) ── other-sundries

And a mid-sized compartment:

│  ├──────89,736 B (00.13%) -- compartment([System Principal], resource://services-common/utils.js)
│  │      ├──69,632 B (00.10%) -- gc-heap
│  │      │  ├──38,264 B (00.06%) -- arena
│  │      │  │  └──38,264 B (00.06%) ── unused
│  │      │  ├──13,592 B (00.02%) ── sundries
│  │      │  ├───9,216 B (00.01%) -- objects
│  │      │  │   └──9,216 B (00.01%) ── function
│  │      │  └───8,560 B (00.01%) -- shapes
│  │      │      └──8,560 B (00.01%) ── tree
│  │      ├───7,496 B (00.01%) ── script-data
│  │      ├───6,656 B (00.01%) -- shapes-extra
│  │      │   └──6,656 B (00.01%) ── compartment-tables
│  │      └───5,952 B (00.01%) ── other-sundries

Sub-trees with a single child are now common;  I'm going to investigate
whether I can compact them next.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Patch 2: use RegExp.test instead of String.match

RegExp.test is lighterweight than String.match.  This patch was from Nils, r=me.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Patch 3: Set .textNode directly when creating text nodes

This avoids creating a JS-side Node wrapper.  This reduces cum-GC-bytes by about 10%.  Patch was from Nils, r=me.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Patch 4: split up the smaps/ tree

This patch splits the "smaps/" tree into four:  "resident", "pss", "vsize", and "swap".  I like it better this way, but more importantly, it makes the next patch much simpler.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Patch 5: merge each single child node into its parent

This patch avoids unnecessarily showing of tree nesting.  E.g. this:

├────499.00 MB (49.90%) -- a
│    └──499.00 MB (49.90%) -- b
│       └──499.00 MB (49.90%) ── c [3]

becomes this:

├────499.00 MB (49.90%) ── a/b/c [3]

This reduces the number of entries in about:memory by about 20--25% at start-up with Nils patch from bug 754771 applied.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This reduces the number of entries in about:memory by about 20--25% at
> start-up with Nils patch from bug 754771 applied.
Nice!  It also makes about:memory easier to read. ;)

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Patch 6: merge the '++'/'--' spans

Currently for nodes that have children we always generate both a '++' and a
'--', and then we hide the one that's not necessary, and toggle visiblity of 
both when it's clicked.  

This patch changes things so we generate only '++' or '--', and we change 
the text when it's toggled.  This doesn't affect the cumulative-GC-bytes 
noticeably, but it's one less <span> per line so it's gotta reduce memory 
consumption on the C++ side.  The code is also a bit nicer now, esp. since I
could get rid of the kid state constants.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 624651 [details] [diff] [review]
Patch 4: split up the smaps/ tree

>-const kMapTreePaths =
>-  ['smaps/resident', 'smaps/pss', 'smaps/vsize', 'smaps/swap'];
>+const kSmapsTreePrefixes = ['resident/', 'pss/', 'vsize/', 'swap/'];
>+
>+function isSmapsTreePrefix(s)

"resident/foo/bar" isn't really an "smaps tree prefix".

"isSmapsPath" or something?

>@@ -650,46 +652,35 @@ TreeNode.compare = function(a, b) {
> };
> 
> /**
>  * From a table of Reports, builds a tree that mirrors the tree structure that
>  * will be shown as output.
>  *
>  * @param aReports
>  *        The table of Reports, indexed by _unsafePath.
>- * @param aTreeName
>- *        The name of the tree being built.
>+ * @param aTreePrefix
>+ *        The prefix (name) of the tree being built.  Must have '/' on the end.
>  * @return The built tree.
>  */
>-function buildTree(aReports, aTreeName)
>+function buildTree(aReports, aTreePrefix)
> {
>-  // We want to process all reports that begin with |aTreeName|.  First we
>+  assert(aTreePrefix.indexOf('/') == aTreePrefix.length - 1);

I don't particularly care, but it looks like most assertions in this file have
descriptions.

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 624653 [details] [diff] [review]
Patch 5: merge each single child node into its parent

>+  function fillInNonLeafNodes(aT, aCannotMerge)
>   {
>     if (aT._kids.length === 0) {
>       // Leaf node.  Has already been filled in.
>       assert(aT._kind !== undefined, "aT._kind is undefined for leaf node");
>+
>+    } else if (aT._kids.length === 1 && !aCannotMerge) {
>+      // Non-leaf node with one child.  Merge the child with the node to avoid
>+      // redundant entries.
>+      assert(aT._kind === undefined, "aT._kind is defined for non-leaf node");
>+      let kid = aT._kids[0];
>+      let kidBytes = fillInNonLeafNodes(kid);
>+      aT._unsafeName += '/' + kid._unsafeName;
>+      aT._kids = kid._kids;
>+      aT._amount = kid._amount;
>+      aT._description = kid._description;
>+      aT._kind = kid._kind;
>+      if (kid._nMerged) {
>+        aT._nMerged = kid._nMerged
>+      }
>+
>     } else {

/me grumbles about cuddle-else with a linebreak before the else.  uncuddle-else with no extra linebreak is clearly superior.

Anyway, nice patch.  :)

Comment 11

[Nicholas Nethercote [inactive]]

Patch 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e12d6d329af

Comment 12

[Nicholas Nethercote [inactive]]

> /me grumbles about cuddle-else with a linebreak before the else. 

Don't look at the rest of the file, then! :P

Comment 13

[Nicholas Nethercote [inactive]]

Patches 2--6:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c851d643ea5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74787facbf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0e76bd2c8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/95da608768e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ce3744f217

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7e12d6d329af
https://hg.mozilla.org/mozilla-central/rev/3c851d643ea5
https://hg.mozilla.org/mozilla-central/rev/d74787facbf4
https://hg.mozilla.org/mozilla-central/rev/7c0e76bd2c8d
https://hg.mozilla.org/mozilla-central/rev/95da608768e5
https://hg.mozilla.org/mozilla-central/rev/d6ce3744f217