Bug 1005489 - Implement better sub-tree sorting and significance detection in about:memory's diff mode. r=mccr8.
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 28 May 2014 20:37:59 -0700
changeset 185535 a6d5e1bfc6ac7c4b1976dc26b7cfc4bf117405fe
parent 185534 ab7014b5f5ccd0a28fbf0056ab687103df723170
child 185536 3af9693b858d2ce106f423384298192260245069
push id26855
push useremorley@mozilla.com
push dateThu, 29 May 2014 14:35:37 +0000
treeherdermozilla-central@9a72db713446 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1005489
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1005489 - Implement better sub-tree sorting and significance detection in about:memory's diff mode. r=mccr8.
toolkit/components/aboutmemory/content/aboutMemory.js
toolkit/components/aboutmemory/tests/memory-reports-diff1.json
toolkit/components/aboutmemory/tests/memory-reports-diff2.json
toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
--- a/toolkit/components/aboutmemory/content/aboutMemory.js
+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
@@ -1061,50 +1061,77 @@ function TreeNode(aUnsafeName, aUnits, a
   // - _nMerged (only defined if > 1)
   // - _presence (only defined if value is PRESENT_IN_{FIRST,SECOND}_ONLY)
   //
   // Non-leaf TreeNodes have these properties added later:
   // - _kids
   // - _amount
   // - _description
   // - _hideKids (only defined if true)
+  // - _maxAbsDescendant (on-demand, only when gIsDiff is set)
 }
 
 TreeNode.prototype = {
   findKid: function(aUnsafeName) {
     if (this._kids) {
       for (let i = 0; i < this._kids.length; i++) {
         if (this._kids[i]._unsafeName === aUnsafeName) {
           return this._kids[i];
         }
       }
     }
     return undefined;
   },
 
+  // When gIsDiff is false, tree operations -- sorting and determining if a
+  // sub-tree is significant -- are straightforward. But when gIsDiff is true,
+  // the combination of positive and negative values within a tree complicates
+  // things. So for a non-leaf node, instead of just looking at _amount, we
+  // instead look at the maximum absolute value of the node and all of its
+  // descendants.
+  maxAbsDescendant: function() {
+    if (!this._kids) {
+      // No kids? Just return the absolute value of the amount.
+      return max = Math.abs(this._amount);
+    }
+
+    if ('_maxAbsDescendant' in this) {
+      // We've computed this before? Return the saved value.
+      return this._maxAbsDescendant;
+    }
+
+    // Compute the maximum absolute value of all descendants.
+    let max = Math.abs(this._amount);
+    for (let i = 0; i < this._kids.length; i++) {
+      max = Math.max(max, this._kids[i].maxAbsDescendant());
+    }
+    this._maxAbsDescendant = max;
+    return max;
+  },
+
   toString: function() {
     switch (this._units) {
       case UNITS_BYTES:            return formatBytes(this._amount);
       case UNITS_COUNT:
       case UNITS_COUNT_CUMULATIVE: return formatInt(this._amount);
       case UNITS_PERCENTAGE:       return formatPercentage(this._amount);
       default:
         assertInput(false, "bad units in TreeNode.toString");
     }
   }
 };
 
-// Sort TreeNodes first by size, then by name.  This is particularly important
-// for the about:memory tests, which need a predictable ordering of reporters
-// which have the same amount.
+// Sort TreeNodes first by size, then by name.  The latter is important for the
+// about:memory tests, which need a predictable ordering of reporters which
+// have the same amount.
 TreeNode.compareAmounts = function(aA, aB) {
   let a, b;
   if (gIsDiff) {
-    a = Math.abs(aA._amount);
-    b = Math.abs(aB._amount);
+    a = aA.maxAbsDescendant();
+    b = aB.maxAbsDescendant();
   } else {
     a = aA._amount;
     b = aB._amount;
   }
   if (a > b) {
     return -1;
   }
   if (a < b) {
@@ -1229,18 +1256,23 @@ function addHeapUnclassifiedNode(aT, aHe
  *        The tree.
  */
 function sortTreeAndInsertAggregateNodes(aTotalBytes, aT)
 {
   const kSignificanceThresholdPerc = 1;
 
   function isInsignificant(aT)
   {
-    return !gVerbose.checked &&
-           (100 * aT._amount / aTotalBytes) < kSignificanceThresholdPerc;
+    if (gVerbose.checked)
+      return false;
+
+    let perc = gIsDiff
+             ? 100 * aT.maxAbsDescendant() / Math.abs(aTotalBytes)
+             : 100 * aT._amount / aTotalBytes;
+    return perc < kSignificanceThresholdPerc;
   }
 
   if (!aT._kids) {
     return;
   }
 
   aT._kids.sort(TreeNode.compareAmounts);
 
--- a/toolkit/components/aboutmemory/tests/memory-reports-diff1.json
+++ b/toolkit/components/aboutmemory/tests/memory-reports-diff1.json
@@ -8,16 +8,23 @@
     {"process": "P", "path": "explicit/spell-check", "kind": 1, "units": 0, "amount": 4, "description": "Desc."},
     {"process": "P", "path": "explicit/spell-check", "kind": 1, "units": 0, "amount": 5, "description": "Desc."},
 
     {"process": "P", "path": "page-faults-soft", "kind": 2, "units": 2, "amount": 61013, "description": "Desc."},
 
     {"process": "P", "path": "foobar", "kind": 2, "units": 0, "amount": 100, "description": "Desc."},
     {"process": "P", "path": "zero1", "kind": 2, "units": 0, "amount": 0, "description": "Desc."},
 
+    {"process": "P", "path": "a/b", "kind": 2, "units": 0, "amount": 1000000, "description": "Desc."},
+    {"process": "P", "path": "a/c/d", "kind": 2, "units": 0, "amount": 2000000, "description": "Desc."},
+    {"process": "P", "path": "a/c/e", "kind": 2, "units": 0, "amount": 2000000, "description": "Desc."},
+    {"process": "P", "path": "a/c/f", "kind": 2, "units": 0, "amount": 3000000, "description": "Desc."},
+    {"process": "P", "path": "a/c/g", "kind": 2, "units": 0, "amount": 3000000, "description": "Desc."},
+    {"process": "P", "path": "a/h", "kind": 2, "units": 0, "amount": 1000, "description": "Desc."},
+
     {"process": "P2 (pid 22)", "path": "z-moz-nullprincipal:{85e250f3-57ae-46c4-a11e-4176dd39d9c5} 0x1234", "kind": 2, "units": 0, "amount": 33, "description": "Desc."},
 
     {"process": "P3", "path": "p3", "kind": 2, "units": 0, "amount": 55, "description": "Desc."},
 
     {"process": "P5", "path": "p5", "kind": 2, "units": 0, "amount": 0, "description": "Desc."},
 
     {"process": "P7", "path": "p7", "kind": 2, "units": 0, "amount": 5, "description": "Desc."},
 
--- a/toolkit/components/aboutmemory/tests/memory-reports-diff2.json
+++ b/toolkit/components/aboutmemory/tests/memory-reports-diff2.json
@@ -9,16 +9,23 @@
 
     {"process": "P", "path": "page-faults-soft", "kind": 2, "units": 2, "amount": 61013, "description": "Desc."},
 
     {"process": "P", "path": "canvas-2d-pixel-bytes", "kind": 2, "units": 0, "amount": 1000, "description": "Desc."},
     {"process": "P", "path": "canvas-2d-pixel-bytes", "kind": 2, "units": 0, "amount": 2000, "description": "Desc."},
 
     {"process": "P", "path": "foobaz", "kind": 2, "units": 0, "amount": 0, "description": "Desc."},
 
+    {"process": "P", "path": "a/b", "kind": 2, "units": 0, "amount": 2000000, "description": "Desc."},
+    {"process": "P", "path": "a/c/d", "kind": 2, "units": 0, "amount": 2998000, "description": "Desc."},
+    {"process": "P", "path": "a/c/e", "kind": 2, "units": 0, "amount": 1001000, "description": "Desc."},
+    {"process": "P", "path": "a/c/f", "kind": 2, "units": 0, "amount": 3001000, "description": "Desc."},
+    {"process": "P", "path": "a/c/g", "kind": 2, "units": 0, "amount": 3001000, "description": "Desc."},
+    {"process": "P", "path": "a/h", "kind": 2, "units": 0, "amount": 2000, "description": "Desc."},
+
     {"process": "P2 (pid 33)", "path": "z-moz-nullprincipal:{161effaa-c1f7-4010-a08e-e7c9aea01aed} 0x5678", "kind": 2, "units": 0, "amount": 44, "description": "Desc."},
 
     {"process": "P4", "path": "p4", "kind": 2, "units": 0, "amount": 66, "description": "Desc."},
 
     {"process": "P6", "path": "p6", "kind": 2, "units": 0, "amount": 0, "description": "Desc."},
 
     {"process": "P7", "path": "p7/b", "kind": 2, "units": 0, "amount": 3, "description": "Desc."},
     {"process": "P7", "path": "p7/c", "kind": 2, "units": 0, "amount": 4, "description": "Desc."},
--- a/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
+++ b/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
@@ -59,23 +59,23 @@
   function finish()
   {
     mgr.unblockRegistrationAndRestoreOriginalReporters();
     SimpleTest.finish();
   }
 
   // Load the given file into the frame, then copy+paste the entire frame and
   // check that the cut text matches what we expect.
-  function test(aFilename, aFilename2, aExpected, aDumpFirst, aNext) {
+  function test(aFilename, aFilename2, aExpected, aDumpFirst, aVerbose, aNext) {
     let frame = document.getElementById("amFrame");
     frame.focus();
 
     let doc = frame.contentWindow.document;
     let verbosity = doc.getElementById("verbose");
-    verbosity.checked = true;
+    verbosity.checked = aVerbose;
 
     function getFilePath(aFilename) {
       let file = Cc["@mozilla.org/file/directory_service;1"]
                  .getService(Components.interfaces.nsIProperties)
                  .get("CurWorkD", Components.interfaces.nsIFile);
       file.append("chrome");
       file.append("toolkit");
       file.append("components");
@@ -169,17 +169,17 @@
       check();
     }
   }
 
   // Returns a function that chains together multiple test() calls.
   function chain(aPieces) {
     let x = aPieces.shift();
     if (x) {
-      return function() { test(x.filename, x.filename2, x.expected, x.dumpFirst, chain(aPieces)); }
+      return function() { test(x.filename, x.filename2, x.expected, x.dumpFirst, x.verbose, chain(aPieces)); }
     } else {
       return function() { finish(); };
     }
   }
 
   let expectedGood =
 "\
 Explicit-only process\n\
@@ -260,31 +260,113 @@ End of Main Process (pid NNN)\n\
 ";
 
   // This is the output for a malformed data file.
   let expectedBad =
 "\
 Invalid memory report(s): missing 'hasMozMallocUsableSize' property\
 ";
 
-  // This is the output for a diff.
-  let expectedDiff =
+  // This is the output for a non-verbose diff.
+  let expectedDiffNonVerbose =
+"\
+P\n\
+\n\
+WARNING: the 'heap-allocated' memory reporter does not work for this platform and/or configuration. This means that 'heap-unclassified' is not shown and the 'explicit' tree shows less memory than it should.\n\
+Explicit Allocations\n\
+\n\
+-0.01 MB (100.0%) -- explicit\n\
+├──-0.01 MB (99.95%) ── storage/prefixset/goog-phish-shavar\n\
+└──-0.00 MB (00.05%) ++ (2 tiny)\n\
+\n\
+Other Measurements\n\
+\n\
+0.96 MB (100.0%) -- a\n\
+├──0.95 MB (99.80%) ── b\n\
+├──0.00 MB (00.10%) -- c\n\
+│  ├──-0.95 MB (-99.70%) ── e\n\
+│  ├──0.95 MB (99.60%) ── d\n\
+│  └──0.00 MB (00.20%) ++ (2 tiny)\n\
+└──0.00 MB (00.10%) ── h\n\
+\n\
+ 0.00 MB ── canvas-2d-pixel-bytes [2] [+]\n\
+-0.00 MB ── foobar [-]\n\
+\n\
+End of P\n\
+P2 (pid NNN)\n\
+Other Measurements\n\
+\n\
+0.00 MB ── z-moz-nullprincipal:{NNNNNNNN-NNNN-NNNN-NNNN-NNNNNNNNNNNN} 0xNNN\n\
+\n\
+End of P2 (pid NNN)\n\
+P3\n\
+Other Measurements\n\
+\n\
+-0.00 MB ── p3 [-]\n\
+\n\
+End of P3\n\
+P4\n\
+Other Measurements\n\
+\n\
+0.00 MB ── p4 [+]\n\
+\n\
+End of P4\n\
+P7\n\
+Other Measurements\n\
+\n\
+0.00 MB (100.0%) -- p7\n\
+├──0.00 MB (57.14%) ── c [+]\n\
+└──0.00 MB (42.86%) ── b [+]\n\
+\n\
+-0.00 MB ── p7 [-]\n\
+\n\
+End of P7\n\
+P8\n\
+Other Measurements\n\
+\n\
+-0.00 MB (100.0%) -- p8\n\
+└──-0.00 MB (100.0%) -- a\n\
+   ├──-0.00 MB (50.00%) -- b\n\
+   │  ├──-0.00 MB (31.82%) -- c\n\
+   │  │  ├──-0.00 MB (18.18%) ── e [-]\n\
+   │  │  └──-0.00 MB (13.64%) ── d [-]\n\
+   │  ├──-0.00 MB (22.73%) ── f [-]\n\
+   │  └───0.00 MB (-4.55%) ── (fake child) [!]\n\
+   └──-0.00 MB (50.00%) -- g\n\
+      ├──-0.00 MB (31.82%) ── i [-]\n\
+      ├──-0.00 MB (27.27%) ── h [-]\n\
+      └───0.00 MB (-9.09%) ── (fake child) [!]\n\
+\n\
+End of P8\n\
+";
+
+  // This is the output for a verbose diff.
+  let expectedDiffVerbose =
 "\
 P\n\
 \n\
 WARNING: the 'heap-allocated' memory reporter does not work for this platform and/or configuration. This means that 'heap-unclassified' is not shown and the 'explicit' tree shows less memory than it should.\n\
 Explicit Allocations\n\
 \n\
 -10,005 B (100.0%) -- explicit\n\
 ├──-10,000 B (99.95%) ── storage/prefixset/goog-phish-shavar\n\
 ├───────-6 B (00.06%) ── spell-check [2]\n\
 └────────1 B (-0.01%) ── xpcom/category-manager\n\
 \n\
 Other Measurements\n\
 \n\
+1,002,000 B (100.0%) -- a\n\
+├──1,000,000 B (99.80%) ── b\n\
+├──────1,000 B (00.10%) -- c\n\
+│      ├──-999,000 B (-99.70%) ── e\n\
+│      ├──998,000 B (99.60%) ── d\n\
+│      ├──1,000 B (00.10%) ── f\n\
+│      └──1,000 B (00.10%) ── g\n\
+└──────1,000 B (00.10%) ── h\n\
+\n\
 3,000 B ── canvas-2d-pixel-bytes [2] [+]\n\
  -100 B ── foobar [-]\n\
 \n\
 End of P\n\
 P2 (pid NNN)\n\
 Other Measurements\n\
 \n\
 11 B ── z-moz-nullprincipal:{NNNNNNNN-NNNN-NNNN-NNNN-NNNNNNNNNNNN} 0xNNN\n\
@@ -328,26 +410,29 @@ Other Measurements\n\
       ├───-6 B (27.27%) ── h [-]\n\
       └────2 B (-9.09%) ── (fake child) [!]\n\
 \n\
 End of P8\n\
 ";
 
   let frames = [
     // This loads a pre-existing file that is valid.
-    { filename: "memory-reports-good.json", expected: expectedGood, dumpFirst: false },
+    { filename: "memory-reports-good.json", expected: expectedGood, dumpFirst: false, verbose: true },
 
     // This dumps to a file and then reads it back in.
-    { filename: "memory-reports-dumped.json.gz", expected: expectedGood2, dumpFirst: true },
+    { filename: "memory-reports-dumped.json.gz", expected: expectedGood2, dumpFirst: true, verbose: true },
 
     // This loads a pre-existing file that is invalid.
-    { filename: "memory-reports-bad.json",  expected: expectedBad, dumpFirst: false },
+    { filename: "memory-reports-bad.json",  expected: expectedBad, dumpFirst: false, verbose: true },
 
     // This loads a pre-existing diff file.
-    { filename: "memory-reports-diff1.json", filename2: "memory-reports-diff2.json",  expected: expectedDiff, dumpFirst: false }
+    { filename: "memory-reports-diff1.json", filename2: "memory-reports-diff2.json",  expected: expectedDiffNonVerbose, dumpFirst: false, verbose: false },
+
+    // Ditto
+    { filename: "memory-reports-diff1.json", filename2: "memory-reports-diff2.json",  expected: expectedDiffVerbose, dumpFirst: false, verbose: true }
   ];
 
   SimpleTest.waitForFocus(chain(frames));
 
   SimpleTest.waitForExplicitFinish();
   ]]>
   </script>
 </window>