Last Comment Bug 755583 - Streamline JS compartment reporters
: Streamline JS compartment reporters
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on: 720595
Blocks: 754771
  Show dependency treegraph
 
Reported: 2012-05-15 17:11 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-05-19 19:37 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1: introduced "sundries" memory reports for compartments (17.41 KB, patch)
2012-05-16 21:53 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
luke: review+
Details | Diff | Review
Patch 2: use RegExp.test instead of String.match (1.74 KB, patch)
2012-05-16 21:54 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
n.nethercote: review+
Details | Diff | Review
Patch 3: Set .textNode directly when creating text nodes (937 bytes, patch)
2012-05-16 21:56 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
n.nethercote: review+
Details | Diff | Review
Patch 4: split up the smaps/ tree (16.93 KB, patch)
2012-05-16 21:58 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 5: merge each single child node into its parent (22.07 KB, patch)
2012-05-16 22:01 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 6: merge the '++'/'--' spans (8.27 KB, patch)
2012-05-16 22:54 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-15 17:11:34 PDT
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 [:njn] (on vacation until July 11) 2012-05-15 22:35:23 PDT
> 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 [:njn] (on vacation until July 11) 2012-05-16 21:53:40 PDT
Created attachment 624647 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2012-05-16 21:54:44 PDT
Created attachment 624648 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2012-05-16 21:56:32 PDT
Created attachment 624649 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2012-05-16 21:58:20 PDT
Created attachment 624651 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2012-05-16 22:01:20 PDT
Created attachment 624653 [details] [diff] [review]
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 (PTO-ish through 6-29) [:mccr8] 2012-05-16 22:02:33 PDT
(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 [:njn] (on vacation until July 11) 2012-05-16 22:54:07 PDT
Created attachment 624662 [details] [diff] [review]
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) 2012-05-17 09:05:49 PDT
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) 2012-05-17 09:20:32 PDT
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 [:njn] (on vacation until July 11) 2012-05-17 17:37:31 PDT
Patch 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e12d6d329af
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-17 17:55:08 PDT
> /me grumbles about cuddle-else with a linebreak before the else. 

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

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