Bug 1005489 - Implement better sub-tree sorting and significance detection in about:memory's diff mode. r=mccr8.

--HG--
extra : rebase_source : 5a157d0ed2f099550aa2b803ac4f6f172b59ba44

toolkit/components/aboutmemory/content/aboutMemory.js

@@ -1066,6 +1066,7 @@ function TreeNode(aUnsafeName, aUnits, aIsDegenerate)
   // - _amount
   // - _description
   // - _hideKids (only defined if true)
+  // - _maxAbsDescendant (on-demand, only when gIsDiff is set)
 }
 
 TreeNode.prototype = {
@@ -1080,6 +1081,32 @@ TreeNode.prototype = {
     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);
@@ -1092,14 +1119,14 @@ TreeNode.prototype = {
   }
 };
 
-// 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;
@@ -1234,8 +1261,13 @@ function sortTreeAndInsertAggregateNodes(aTotalBytes, aT)
 
   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) {

toolkit/components/aboutmemory/tests/memory-reports-diff1.json

@@ -13,6 +13,13 @@
     {"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."},

toolkit/components/aboutmemory/tests/memory-reports-diff2.json

@@ -14,6 +14,13 @@
 
     {"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."},

toolkit/components/aboutmemory/tests/test_aboutmemory3.xul

@@ -64,13 +64,13 @@
 
   // 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"]
@@ -174,7 +174,7 @@
   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(); };
     }
@@ -265,8 +265,81 @@ End of Main Process (pid NNN)\n\
 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\
@@ -280,6 +353,15 @@ Explicit Allocations\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\
@@ -333,16 +415,19 @@ 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));