Last Comment Bug 757299 - Streamline about:memory yet again
: Streamline about:memory yet again
Status: RESOLVED FIXED
:
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:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-21 17:56 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-05-28 15:13 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1: optional-kids (7.29 KB, patch)
2012-05-21 17:58 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 2: better-flipBack (1.01 KB, patch)
2012-05-21 17:59 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 3: "below this one" (1.07 KB, patch)
2012-05-21 18:01 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 4: merge-newline (4.67 KB, patch)
2012-05-21 18:06 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 5: remove-Report-objects (21.86 KB, patch)
2012-05-21 18:11 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 6: better-path-concat (4.95 KB, patch)
2012-05-21 18:13 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 7: rm-_kind (12.50 KB, patch)
2012-05-21 18:16 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 8: tweak-perc (1.59 KB, patch)
2012-05-21 18:17 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 9: better-treelines (8.21 KB, patch)
2012-05-21 18:20 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 10: rm-kind-from-desc (2.75 KB, patch)
2012-05-21 18:22 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-21 17:56:03 PDT
Following on from 755583.

Here are 10 patches, most of which reduce about:memory's memory consumption, and a couple which are just code clean-ups.  The followings stats show the memory reductions;  "combined" is the most important one.

                    cum-GC      string      object      heap-unc    combined
original            10.5        4.7         7.9         12.2        24.7
optional-kids       10.1        4.7         7.5         11.9        24.1
better-flipBack      9.6        4.7         7.0         11.9        23.5
"below-this-one"     9.6        4.5         7.0         11.9        23.4
merge-newline        9.5        4.7         6.8         11.5        23.0
rm-Report-objects    8.2        4.8         5.3         11.4        21.5
better-path-concat   7.9        4.7         5.4         11.4        21.5
rm-_kind             7.9        4.7         5.4         11.4        21.5
tweak-perc           7.8        4.4         5.6         11.4        21.4
better-treeLine      8.0        5.6         3.9         11.4        20.8
rm-kind-from-desc    8.0        4.7         3.9         11.4        20.0
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 17:58:21 PDT
Created attachment 625835 [details] [diff] [review]
Patch 1: optional-kids

This patch removes the empty |kids| array from leaf TreeNodes.  It also slightly improves the merging of single child nodes into their parent.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 17:59:56 PDT
Created attachment 625836 [details] [diff] [review]
Patch 2: better-flipBack

This patch avoids creating a new string in the common case where the given string doesn't contain a '\'.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:01:49 PDT
Created attachment 625837 [details] [diff] [review]
Patch 3: "below this one"

This makes the description on non-leaf nodes more generic.  No loss, IMO.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:06:18 PDT
Created attachment 625838 [details] [diff] [review]
Patch 4: merge-newline

This patch puts the newline at the end of each line in the last <span> on the line, instead of in a separate text node.  Saves one text node per line.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:11:02 PDT
Created attachment 625839 [details] [diff] [review]
Patch 5: remove-Report-objects

Currently we (1) create a Report object for each memory report, (2) build the tree from the Report objects, and (3) build the DOM from the tree.

This patch merges steps (1) and (2), so Reporter objects are no longer necessary.  It's a 40 line reduction in code.  And it saves jlebar from having to worry whether I'm using "report" or "Report" appropriately in comments :)

(BTW, merging step (1+2) with step (3) is theoretically possible but would be awful, so I have no intention of doing it.)
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:13:03 PDT
Created attachment 625840 [details] [diff] [review]
Patch 6: better-path-concat

This makes the creation of paths from the tree a bit nicer.  Not really a memory win, but it will make it easier to remove the safe/unsafe path distinction later.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:16:27 PDT
Created attachment 625841 [details] [diff] [review]
Patch 7: rm-_kind

This removes the TreeNode._kids field.  It had two uses:

- For adding "(Heap)" or "(Non-heap)" to descriptions.  This is now done right at the start.

- For measuring HEAP usage in the explicit tree.  This is now done earlier.

Again, not a memory win, but I like that it allows some assertions to be removed.
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:17:59 PDT
Created attachment 625842 [details] [diff] [review]
Patch 8: tweak-perc

This is a slightly more efficient way to construct the percentage strings.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:20:36 PDT
Created attachment 625845 [details] [diff] [review]
Patch 9: better-treelines

This is a more efficient way to generate the treelines.  Easier to understand, too.
Comment 10 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 18:22:36 PDT
Created attachment 625847 [details] [diff] [review]
Patch 10: rm-kind-from-desc

This removes the prepending of "(Heap)" or "(Non-heap)" from explicit tree descriptions.  I don't think it adds much;  if it's really useful in certain descriptions we could put it in the description itself.
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 19:10:53 PDT
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Created attachment 625839 [details] [diff] [review]
> Patch 5: remove-Report-objects
> 
> This patch merges steps (1) and (2), so Reporter objects are no longer
> necessary.

I forgot to say:  like patch 6, this will make removing the safe/unsafe distinction easier.
Comment 12 Justin Lebar (not reading bugmail) 2012-05-22 08:05:42 PDT
Comment on attachment 625835 [details] [diff] [review]
Patch 1: optional-kids

I've seen JS strict mode warnings about "referencing undefined property", but now I can't re-create them.  It may be preferable to do |if ('kids' in a)| instead of |if (a.kids === undefined)|.

But we should also be consistent; I'd be OK with picking either of those (or simply |if (a.kids)|), so long as whatever we pick doesn't warn.

> TreeNode.prototype = {
>   findKid: function(aUnsafeName) {
>-    for (let i = 0; i < this._kids.length; i++) {
>-      if (this._kids[i]._unsafeName === aUnsafeName) {
>-        return this._kids[i];
>+    if (this._kids) {
>+      for (let i = 0; i < this._kids.length; i++) {
>+        if (this._kids[i]._unsafeName === aUnsafeName) {
>+          return this._kids[i];
>+        }
>       }
>     }
>     return undefined;
>   },
> 
>   toString: function() {
>     return formatBytes(this._amount);
>   }
>@@ -700,16 +702,19 @@ function buildTree(aReports, aTreePrefix
>       let u = t;
>       for (let i = 0; i < unsafeNames.length; i++) {
>         let unsafeName = unsafeNames[i];
>         let uMatch = u.findKid(unsafeName);
>         if (uMatch) {
>           u = uMatch;
>         } else {
>           let v = new TreeNode(unsafeName);
>+          if (!u._kids) {
>+            u._kids = [];
>+          }
>           u._kids.push(v);
>           u = v;
>         }
>       }
>       // Fill in extra details in the leaf node from the Report object.
>       u._amount = r._amount;
>       u._description = r._description;
>       u._kind = r._kind;
>@@ -730,34 +735,41 @@ function buildTree(aReports, aTreePrefix
> 
>   // Using falseRoot makes the above code simpler.  Now discard it, leaving
>   // aTreePrefix at the root.
>   t = t._kids[0];
> 
>   // Next, fill in the remaining properties bottom-up.
>   function fillInNonLeafNodes(aT, aCannotMerge)
>   {
>-    if (aT._kids.length === 0) {
>+    if (!aT._kids) {
>       // 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;
>+      if (kid._kids !== undefined) {
>+        aT._kids = kid._kids;
>+      } else {
>+        delete aT._kids;
>+      }

>       aT._amount = kid._amount;
>       aT._description = kid._description;
>-      aT._kind = kid._kind;
>-      if (kid._nMerged) {
>+      if (kid._kind !== undefined) {
>+        aT._kind = kid._kind;
>+      }
>+      if (kid._nMerged !== undefined) {

This one needs to still be !== undefined, so I guess the other ones are OK.
Comment 13 Justin Lebar (not reading bugmail) 2012-05-22 08:19:54 PDT
Comment on attachment 625838 [details] [diff] [review]
Patch 4: merge-newline

>-function appendMrNameSpan(aP, aKind, aSep, aDescription, aUnsafeName,
>+function appendMrNameSpan(aP, aKind, aDescription, aUnsafeName,
>                           aIsInvalid, aNMerged)
> {
>-  appendElementWithText(aP, "span", "mrSep", aSep);
>-
>-  let nameSpan = appendElementWithText(aP, "span", "mrName",
>-                                       flipBackslashes(aUnsafeName));
>+  let safeName = flipBackslashes(aUnsafeName);
>+  if (!aIsInvalid && !aNMerged) {
>+    safeName += "\n";
>+  }
>+  let nameSpan = appendElementWithText(aP, "span", "mrName", safeName);
>   nameSpan.title = kindToString(aKind) + aDescription;
> 
>   if (aIsInvalid) {
>-    let noteSpan = appendElementWithText(aP, "span", "mrNote", " [?!]");
>+    let noteText = " [?!]";
>+    if (!aNMerged) {
>+      noteText += "\n";
>+    }

Save a string op here by doing

  let noteText = aNMerged ? " [?!]\n" : " [?!]";

?  Although I guess it doesn't matter because we don't hit this often.
Comment 14 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-23 16:38:19 PDT
> It may be preferable to do |if ('kids' in a)| instead of |if (a.kids === undefined)|.

That is nicer.  But there are lots of places where I test for |undefined|;  I'll rip them out in a final patch.

> >+      if (kid._nMerged !== undefined) {
> 
> This one needs to still be !== undefined

Nope;  |"_nMerged" in kid| would be fine.
Comment 15 Justin Lebar (not reading bugmail) 2012-05-24 10:28:24 PDT
> > This one needs to still be !== undefined
> 
> Nope;  |"_nMerged" in kid| would be fine.

Yeah, I'm not sure why I thought otherwise.  I think I may have thought that _nMerged could be 0, but it looks like you avoid that.
Comment 16 Justin Lebar (not reading bugmail) 2012-05-25 14:33:42 PDT
Comment on attachment 625839 [details] [diff] [review]
Patch 5: remove-Report-objects

Cool.

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js
>--- a/toolkit/components/aboutmemory/content/aboutMemory.js
>+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
>@@ -459,24 +459,28 @@ function updateAboutMemory()
> 
>   // First, clear the page contents.  Necessary because updateAboutMemory()
>   // might be called more than once due to the "child-memory-reporter-update"
>   // observer.
>   let body = clearBody();
> 
>   // Generate output for one process at a time.  Always start with the
>   // Main process.
>-  let reportsByProcess = getReportsByProcess(mgr);
>+  let treesByProcess = {}, othersByProcess = {};
>+  getTreesAndOthersByProcess(mgr, treesByProcess, othersByProcess);
>+
>   let hasMozMallocUsableSize = mgr.hasMozMallocUsableSize;
>-  appendProcessReportsElements(body, "Main", reportsByProcess["Main"],
>-                               hasMozMallocUsableSize);
>-  for (let process in reportsByProcess) {

I feel like the comment should go right here.

>+  appendProcessAboutMemoryElements(body, "Main", treesByProcess["Main"],
>+                                   othersByProcess["Main"],
>+                                   hasMozMallocUsableSize);
>+  for (let process in treesByProcess) {
>     if (process !== "Main") {
>-      appendProcessReportsElements(body, process, reportsByProcess[process],
>-                                   hasMozMallocUsableSize);
>+      appendProcessAboutMemoryElements(body, process, treesByProcess[process],
>+                                       othersByProcess[process],
>+                                       hasMozMallocUsableSize);
>     }
>   }


>   function handleReport(aProcess, aUnsafePath, aKind, aUnits, aAmount,
>                         aDescription)
>   {
>     ...
>+    if (aUnsafePath.indexOf('/') !== -1) {
>+      // Tree report.  Get the tree for the process, creating it if necessary.
>+      // All the trees for each process ("explicit", "vsize", etc") are stored

etc"

>-function buildTree(aReports, aTreePrefix)
>+function fillInTree(aTreeOfTrees, aTreePrefix)
>   ...
>+  // There should always be an "explicit/" tree.  But smaps trees may not be
>+  // present;  if that happens, bail.
>+  let t = aTreeOfTrees.findKid(aTreePrefix.replace(/\//g, ''));
>+  if (!t) {
>+    assert(aTreePrefix !== 'explicit/', "missing explicit tree");
>     return null;
>   }

We're bailing if there's no explicit tree, but that's not what the comment says.

Sorry for the wait, Nick.
Comment 17 Justin Lebar (not reading bugmail) 2012-05-25 14:49:48 PDT
Comment on attachment 625841 [details] [diff] [review]
Patch 7: rm-_kind

>@@ -635,18 +642,30 @@ function getTreesAndOthersByProcess(aMgr
>     
>       if (u._amount) {
>         // Duplicate!  Sum the values and mark it as a dup.
>         u._amount += aAmount;
>         u._nMerged = u._nMerged ? u._nMerged + 1 : 2;
>       } else {
>         // New leaf node.  Fill in extra details node from the report.
>         u._amount = aAmount;
>-        u._description = aDescription;
>-        u._kind = aKind;
>+        if (unsafeNames[0] === "explicit") {
>+          u._description = kindToString(aKind) + aDescription;
>+        } else {
>+          // We don't want to show '(Non-heap)' on an smaps tree because
>+          // the whole tree is non-heap.
>+          u._description = aDescription;
>+        }
>+      }
>+
>+      if (unsafeNames[0] === "explicit" && aKind == KIND_HEAP) {
>+        if (!aHeapTotalByProcess[process]) {
>+          aHeapTotalByProcess[process] = 0;
>+        }
>+        aHeapTotalByProcess[process] += aAmount;
>       }

Can we write a JS equivalent of Python's defaultdict yet?
http://docs.python.org/library/collections.html#collections.defaultdict

>-function fixUpExplicitTree(aT, aOthers)
>+function fixUpExplicitTree(aT, aOthers, aHeapTotal)
> {
>   // A special case:  compute the derived "heap-unclassified" value.  Don't
>   // mark "heap-allocated" when we get its size because we want it to appear
>   // in the "Other Measurements" list.
>   let heapAllocatedReport = aOthers["heap-allocated"];
>   if (heapAllocatedReport === undefined)
>     return false;
> 
>   let heapAllocatedBytes = heapAllocatedReport._amount;
>   let heapUnclassifiedT = new TreeNode("heap-unclassified");
>+  heapUnclassifiedT._amount = heapAllocatedBytes - aHeapTotal;
>   gHeapUnc = heapUnclassifiedT._amount;
>+  heapUnclassifiedT._description = kindToString(KIND_HEAP) +
>       "Memory not classified by a more specific reporter. This includes " +
>       "slop bytes due to internal fragmentation in the heap allocator " +
>       "(caused when the allocator rounds up request sizes).";
>   aT._kids.push(heapUnclassifiedT);
>   aT._amount += heapUnclassifiedT._amount;
>   return true;
> }

Now that I can easily see this whole function, it occurs to me that we're not
really "fixing up" anything here anymore; we're just adding the
heap-unclassified node.
Comment 18 Justin Lebar (not reading bugmail) 2012-05-25 15:18:15 PDT
Comment on attachment 625845 [details] [diff] [review]
Patch 9: better-treelines

>-const kHorizontal       = "\u2500",
>-      kVertical         = "\u2502",
>-      kUpAndRight       = "\u2514",
>-      kVerticalAndRight = "\u251c",
>-      kNoKidsSep        = " \u2500\u2500 ",
>-      kHideKidsSep      = " ++ ",
>-      kShowKidsSep      = " -- ";
>+const kHorizontal                   = "\u2500",
>+      kVertical                     = "\u2502",
>+      kUpAndRight                   = "\u2514",
>+      kUpAndRight_Right_Right       = "\u2514\u2500\u2500",
>+      kVerticalAndRight             = "\u251c",
>+      kVerticalAndRight_Right_Right = "\u251c\u2500\u2500",
>+      kVerticalAndSpace_Space_Space = "\u2502  ",
>+      kNoKidsSep                    = " \u2500\u2500 ",
>+      kHideKidsSep                  = " ++ ",
>+      kShowKidsSep                  = " -- ";

I think you're forgetting kUpUpDownDownLeftRightLeftRightBA.

>@@ -1278,52 +1281,50 @@ function appendTreeElements(aPOuter, aT,
>    * Appends the elements for a particular tree, without a heading.
>    *
>    * @param aP
>    *        The parent DOM node.
>    * @param aUnsafeNames
>    *        An array of the names forming the path to aT.
>    * @param aT
>    *        The tree.
>-   * @param aBaseIndentText
>-   *        The base text of the indent, which may be augmented within the
>-   *        function.
>-   * @param aIndentGuide
>-   *        Records what indentation is required for this tree.  It has one
>-   *        entry per level of indentation.  For each entry, ._isLastKid
>-   *        records whether the node in question is the last child, and
>-   *        ._depth records how many chars of indentation are required.
>+   * @param aTreelineText1
>+   *        The first part of the treeline for this entry and this entry's
>+   *        children.
>+   * @param aTreelineText2a
>+   *        The second part of the treeline for this entry.
>+   * @param aTreelineForChildrenText2
>+   *        The second part of the treeline for this entry's children.

Wrong param name.

>    * @param aParentStringLength
>    *        The length of the formatted byte count of the top node in the tree.
>    */
>-  function appendTreeElements2(aP, aUnsafeNames, aT, aIndentGuide,
>-                               aBaseIndentText, aParentStringLength)
>+  function appendTreeElements2(aP, aUnsafeNames, aT, aTreelineText1,
>+                               aTreelineText2a, aTreelineText2b,
>+                               aParentStringLength)
>   {
>-    function repeatStr(aA, aC, aN)
>+    function appendN(aS, aC, aN)
>     {
>       for (let i = 0; i < aN; i++) {
>-        aA.push(aC);
>+        aS += aC;
>       }
>+      return aS;
>     }

It might be a premature optimization, but I imagine this could use less memory.
(e.g. we could memoize the string-to-append.)
Comment 19 Justin Lebar (not reading bugmail) 2012-05-25 15:20:03 PDT
Comment on attachment 625847 [details] [diff] [review]
Patch 10: rm-kind-from-desc

I agree that "(Heap)" and "(Non-heap)" aren't adding much.
Comment 20 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-26 19:12:08 PDT
> Can we write a JS equivalent of Python's defaultdict yet?
> http://docs.python.org/library/collections.html#collections.defaultdict

Assuming I've understood your point correctly... ECMAScript 6 will have a new "Map" type, but it's only just been implemented in SpiderMonkey and subject to change, so I don't want to use it yet:

> >-  function appendTreeElements2(aP, aUnsafeNames, aT, aIndentGuide,
> >-                               aBaseIndentText, aParentStringLength)
> >+  function appendTreeElements2(aP, aUnsafeNames, aT, aTreelineText1,
> >+                               aTreelineText2a, aTreelineText2b,
> >+                               aParentStringLength)
> >   {
> >-    function repeatStr(aA, aC, aN)
> >+    function appendN(aS, aC, aN)
> >     {
> >       for (let i = 0; i < aN; i++) {
> >-        aA.push(aC);
> >+        aS += aC;
> >       }
> >+      return aS;
> >     }
> 
> It might be a premature optimization, but I imagine this could use less
> memory.
> (e.g. we could memoize the string-to-append.)

I measured, the two versions were basically the same.
Comment 21 Justin Lebar (not reading bugmail) 2012-05-27 04:17:35 PDT
> ECMAScript 6 will have a new "Map" type,

From a brief perusal, I don't think Map is what I mean here.  The key feature of Map seems to be that you can use any object as a key.

What I mean is a normal dictionary with the caveat that when you do

  dict["foo"]

and "foo" is not in the dictionary, instead of getting undefined, you get some value you specified when you created the map.

But anyway, it's not a specific comment to your patches; we don't need to change anything here.  If I'm feeling ambitious, I could change it as a followup.
Comment 22 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-27 18:23:14 PDT
> What I mean is a normal dictionary with the caveat that when you do
> 
>   dict["foo"]
> 
> and "foo" is not in the dictionary, instead of getting undefined, you get
> some value you specified when you created the map.

Oh, so that's what the "default" in "defaultdict" means.  Yes, I don't think JS has that.

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