Closed Bug 757299 Opened 12 years ago Closed 12 years ago

Streamline about:memory yet again

Categories

(Toolkit :: about:memory, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(10 files)

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
This patch removes the empty |kids| array from leaf TreeNodes.  It also slightly improves the merging of single child nodes into their parent.
Attachment #625835 - Flags: review?(justin.lebar+bug)
This patch avoids creating a new string in the common case where the given string doesn't contain a '\'.
Attachment #625836 - Flags: review?(justin.lebar+bug)
This makes the description on non-leaf nodes more generic.  No loss, IMO.
Attachment #625837 - Flags: review?(justin.lebar+bug)
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.
Attachment #625838 - Flags: review?(justin.lebar+bug)
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.)
Attachment #625839 - Flags: review?(justin.lebar+bug)
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.
Attachment #625840 - Flags: review?(justin.lebar+bug)
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.
Attachment #625841 - Flags: review?(justin.lebar+bug)
This is a slightly more efficient way to construct the percentage strings.
Attachment #625842 - Flags: review?(justin.lebar+bug)
This is a more efficient way to generate the treelines.  Easier to understand, too.
Attachment #625845 - Flags: review?(justin.lebar+bug)
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.
Attachment #625847 - Flags: review?(justin.lebar+bug)
Attachment #625841 - Attachment description: Patch 7: rm-_kids → Patch 7: rm-_kind
(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 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.
Attachment #625835 - Flags: review?(justin.lebar+bug) → review+
Attachment #625836 - Flags: review?(justin.lebar+bug) → review+
Attachment #625837 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #625838 - Flags: review?(justin.lebar+bug) → review+
> 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.
> > 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 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.
Attachment #625839 - Flags: review?(justin.lebar+bug) → review+
Attachment #625840 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #625841 - Flags: review?(justin.lebar+bug) → review+
Attachment #625842 - Flags: review?(justin.lebar+bug) → review+
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.)
Attachment #625845 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 625847 [details] [diff] [review]
Patch 10: rm-kind-from-desc

I agree that "(Heap)" and "(Non-heap)" aren't adding much.
Attachment #625847 - Flags: review?(justin.lebar+bug) → review+
> 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.
> 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.
> 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.
You need to log in before you can comment on or make changes to this bug.