Last Comment Bug 722972 - Reduce about:memory perturbation
: Reduce about:memory perturbation
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 724479
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 20:27 PST by Nicholas Nethercote [:njn]
Modified: 2012-02-21 00:14 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
cleanups (1.33 KB, patch)
2012-02-06 20:07 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
avoid-slice (687 bytes, patch)
2012-02-06 20:08 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review-
Details | Diff | Review
rm-getBytes (5.66 KB, patch)
2012-02-06 20:11 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
hasKids-only (6.36 KB, patch)
2012-02-06 20:17 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
redo-indent (5.08 KB, patch)
2012-02-06 20:20 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
repeatStr (2.91 KB, patch)
2012-02-06 20:24 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
formatInt (2.03 KB, patch)
2012-02-06 20:25 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
avoid-slice v2 (1.02 KB, patch)
2012-02-07 21:43 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review
formatInt v2 (2.22 KB, patch)
2012-02-07 22:15 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-01-31 20:27:41 PST
In order to generate about:memory, we have to allocate some memory.  This obviously perturbs the measurement, particular if you hit the update/GC/CC/MMU buttons at the bottom.

The aim of this bug is to reduce this perturbation.  I'll use the methodology from bug 722969 comment 0:

> I found a methodology for evaluating this that wasn't too bad.  If you hit
> the "update" button on about:memory?verbose over and over again, the
> counters bounce around, but some of them tend to increase by the same amount
> frequently.  I instrumented aboutMemory.js to show the delta for
> "js-string-total", "js-objects-total" and "heap-unclassified", all of which
> have this behaviour, and together which they account for much of the memory
> allocated by about:memory, AFAICT.  (I played with this a *lot*, trying 
> various metrics before settling on this one.)

I've made some progress on this, but I need to clean the changes up and break them into separate patches.  More soon.
Comment 1 Nicholas Nethercote [:njn] 2012-02-06 20:06:00 PST
Here are the numbers for the forthcoming patches, where "base" is the starting point.

                | string-total    obj-total   heap-unc        combined
-----------------------------------------------------------------------------
base            | 5.57MB          3.40MB      4.94MB          13.96MB
avoid-slice     | 4.58MB          3.60MB      4.96MB          13.13MB
rm-getBytes     | 3.91MB          3.28MB      4.96MB          12.14MB
hasKids-only    | 3.31MB          3.11MB      4.96MB          11.37MB
redo-indent     | 2.85MB          2.67MB      4.28MB           9.78MB
repeatStr       | 1.77MB          3.19MB      4.32MB           9.28MB
formatInt       | 1.37MB          3.32MB      4.27MB           8.96MB
Comment 2 Nicholas Nethercote [:njn] 2012-02-06 20:07:17 PST
Created attachment 594895 [details] [diff] [review]
cleanups

This first patch doesn't affect memory allocation, it just removes some dead variables and parameters.
Comment 3 Nicholas Nethercote [:njn] 2012-02-06 20:08:26 PST
Created attachment 594896 [details] [diff] [review]
avoid-slice

This patch avoids using String.slice(), which modifies the string, just when doing some matching.  It also avoids a concatenation.
Comment 4 Nicholas Nethercote [:njn] 2012-02-06 20:11:55 PST
Created attachment 594897 [details] [diff] [review]
rm-getBytes

This patch initializes all fields in leaf TreeNodes immediately, rather than waiting.  This saves memory by avoiding the need to construct a path in fillInNonLeafNodes().  It also allows getBytes() and getUnsafeDescription() to be removed, which is a step towards eliminating the safe/unsafe stuff.
Comment 5 Nicholas Nethercote [:njn] 2012-02-06 20:17:52 PST
Created attachment 594898 [details] [diff] [review]
hasKids-only

This patch avoids doing some unnecessary work when |hasKids| is false.
Comment 6 Nicholas Nethercote [:njn] 2012-02-06 20:20:28 PST
Created attachment 594901 [details] [diff] [review]
redo-indent

This patch avoids generating identical indent strings for each child of a node.
Comment 7 Nicholas Nethercote [:njn] 2012-02-06 20:24:11 PST
Created attachment 594902 [details] [diff] [review]
repeatStr

This patch changes repeatStr to build up an array of strings instead of a string.  The array can be join()ed when ready.
Comment 8 Nicholas Nethercote [:njn] 2012-02-06 20:25:37 PST
Created attachment 594903 [details] [diff] [review]
formatInt

Use Array.join() in formatInt().  Also include the unit in the join() when appropriate.
Comment 9 Justin Lebar (not reading bugmail) 2012-02-07 05:27:02 PST
Comment on attachment 594896 [details] [diff] [review]
avoid-slice

string.match() is for regular expressions.  I presume this treats aTreeName as a regexp, not a string?  In which case it appears you want /^(regexp-escape aTreeName)\//, but I don't know how to do that.

Even if you could build a regexp, I don't think you want to, because we compile those, and that takes memory.

Looks like you really want string.startsWith, which we don't have.  So you're stuck with indexOf == 0, which is probably fine, or writing your own String.prototype.startsWith function.
Comment 10 Justin Lebar (not reading bugmail) 2012-02-07 05:46:35 PST
Comment on attachment 594897 [details] [diff] [review]
rm-getBytes

Is defining all the properties on Reporter in its constructor a good idea?  I imagine the JS engine would be able to allocate those objects more efficiently, but what do I know?

If so, you could define this._done = false and this.nMerged = 0 in the Reporter's constructor.

You could do the same thing for TreeNode, if it helps.
Comment 11 Justin Lebar (not reading bugmail) 2012-02-07 05:57:32 PST
Comment on attachment 594897 [details] [diff] [review]
rm-getBytes

>@@ -216,7 +216,7 @@ function Reporter(aUnsafePath, aKind, aU
>   this._amount      = aAmount;
>   this._unsafeDescription = aUnsafeDesc;
>   // this._nMerged is only defined if > 1
>-  // this._done is defined when getBytes is called
>+  // this._done is defined when the reporter's amount is read
> }

I never understood about:memory's not-defining-properties-until-things-happen
approach, but see comment 10.   At least the comment could
s/defined/defined and set to true/.

> Reporter.prototype = {
>@@ -423,16 +423,16 @@ function TreeNode(aUnsafeName)
>   // Nb: _units is not needed, it's always UNITS_BYTES.
>   this._unsafeName = aUnsafeName;
>   this._kids = [];
>-  // All TreeNodes have these properties added later:
>+  // Leaf TreeNodes have these properties added immediately:
>   // - _amount (which is never |kUnknown|)
>   // - _unsafeDescription

Immediately when?  Not here in the constructor...

>@@ -552,7 +550,7 @@ function buildTree(aReporters, aTreeName
>     return aT._amount;
>   }
> 
>-  fillInTree(t, "");
>+  fillInNonLeafNodes(t, "");

Extra param?  (I wish I were surprised that JS doesn't complain.)
Comment 12 Justin Lebar (not reading bugmail) 2012-02-07 09:14:27 PST
Comment on attachment 594898 [details] [diff] [review]
hasKids-only

>+// Possible states for kids.
>+const kNoKids   = 44;
>+const kHideKids = 55;
>+const kShowKids = 66;

Why aren't these 0, 1, and 2?
Comment 13 Justin Lebar (not reading bugmail) 2012-02-07 09:24:44 PST
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Created attachment 594902 [details] [diff] [review]
> repeatStr
> 
> This patch changes repeatStr to build up an array of strings instead of a
> string.  The array can be join()ed when ready.

It looks like it builds up an array of strings and also builds a string.  Is the JS engine smart enough to inline this function and then perform dead-code elimination so as not to build up the string?  I'd be impressed...

Anyway, rather than build up an array of " " for each memory reporter and then create a string, shouldn't we create the strings "", " ", "  ", "   ", as required, store them, and then return them as needed?
Comment 14 Justin Lebar (not reading bugmail) 2012-02-07 09:26:05 PST
The change in comment 13 would affect the repeat-str and redo-indent patches.  Rather than pass around a base indent string in repeat-str, you'd just pass around a number which you convert to the indent string at the very end.
Comment 15 Justin Lebar (not reading bugmail) 2012-02-07 09:30:41 PST
Comment on attachment 594903 [details] [diff] [review]
formatInt

I'm surprised that this uses less memory than the string code!

Could you please add a comment to formatInt saying we use an array because it uses less memory?
Comment 16 Justin Lebar (not reading bugmail) 2012-02-07 09:31:16 PST
Comment on attachment 594901 [details] [diff] [review]
redo-indent

r- pending comment 13.  Please re-flag if you think comment 13 is a bad idea, for some reason.
Comment 17 Nicholas Nethercote [:njn] 2012-02-07 21:43:08 PST
Created attachment 595314 [details] [diff] [review]
avoid-slice v2

String.match() converts its arg to a RegExp if necessary.  But you're right that using indexOf() is better, it shaved about 0.2MB off the objects count.
Comment 18 Nicholas Nethercote [:njn] 2012-02-07 21:49:59 PST
> If so, you could define this._done = false and this.nMerged = 0 in the
> Reporter's constructor.

I tried it, doesn't make any difference.
Comment 19 Nicholas Nethercote [:njn] 2012-02-07 21:56:25 PST
> I never understood about:memory's not-defining-properties-until-things-happen
> approach, but see comment 10.   At least the comment could
> s/defined/defined and set to true/.

From my understanding of how the JS engine works, adding more properties might lead to increased memory consumption, though it depends on various circumstances.  I updated the comment.


> >+  // Leaf TreeNodes have these properties added immediately:
> >   // - _amount (which is never |kUnknown|)
> >   // - _unsafeDescription
> 
> Immediately when?  Not here in the constructor...

I changed it to "immediately after construction".

> 
> >@@ -552,7 +550,7 @@ function buildTree(aReporters, aTreeName
> >     return aT._amount;
> >   }
> > 
> >-  fillInTree(t, "");
> >+  fillInNonLeafNodes(t, "");
> 
> Extra param?  (I wish I were surprised that JS doesn't complain.)

It doesn't complain, and you can even use the Arguments object within the function to access the extra arg.  I've removed it, good catch.
Comment 20 Nicholas Nethercote [:njn] 2012-02-07 21:59:30 PST
> >+// Possible states for kids.
> >+const kNoKids   = 44;
> >+const kHideKids = 55;
> >+const kShowKids = 66;
> 
> Why aren't these 0, 1, and 2?

It's an old habit of mine from C to try to help catch cases where you have bogus enum values due to bugs -- in that case, low values (esp. 0) are most likely, so using unusual higher numbers increases the likelihood of falling through to an unexpected switch case.  But it doesn't make much sense for JS code, I'll change to 0/1/2.
Comment 21 Nicholas Nethercote [:njn] 2012-02-07 22:13:04 PST
> It looks like it builds up an array of strings and also builds a string.

Also builds a string?  I don't see that.  Did you misread the patch?

> Anyway, rather than build up an array of " " for each memory reporter and
> then create a string, shouldn't we create the strings "", " ", "  ", "   ",
> as required, store them, and then return them as needed?

Hmm...

Using an array rather than multiple concats is better because we use ropes for concats, and so multiple concats cause a tree of pending concatenations to be built up -- there's the leaf nodes and then a bunch of non-leaf nodes as well.  In comparison, the array approach just has a single array that points to all the strings -- there are no non-leaf nodes.

Also, all single-char strings are pre-allocated ahead of time.  Putting single-char strings like " " into the array may actually be better than putting multi-char strings like "   " because the latter have to be created.

With these things in mind, reasking for review on these two patches.
Comment 22 Nicholas Nethercote [:njn] 2012-02-07 22:15:31 PST
Created attachment 595317 [details] [diff] [review]
formatInt v2

This version adds the requested comment.
Comment 23 Justin Lebar (not reading bugmail) 2012-02-08 07:34:29 PST
> In comparison, the array approach just has a single array that points to all the strings -- there 
> are no non-leaf nodes.

I think I wasn't clear.

I was saying we could memoize the indent strings.  This would result in a constant number (~10?) of indent strings being allocated, instead of at least one array and one string per memory reporter.

Exactly how you create the memoized strings is up to you.

To be concrete, you could write:

var indentStrings = [];
function getIndent(level) {
  if (!indentStrings[level]) {
    indentStrings[level] = /* generate the appropriate string however you'd like */
  }
  return indentStrings[level];
}

Wouldn't this approach result in many fewer allocations?
Comment 24 Justin Lebar (not reading bugmail) 2012-02-08 08:02:52 PST
> > It looks like it builds up an array of strings and also builds a string.
>
> Also builds a string?  I don't see that.  Did you misread the patch?

Yes, utterly.
Comment 25 Justin Lebar (not reading bugmail) 2012-02-09 13:49:47 PST
Comment on attachment 594901 [details] [diff] [review]
redo-indent

>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
>@@ -1099,60 +1099,53 @@ function appendTreeElements(aPOuter, aT,
>    * Appends the elements for a particular tree, without a heading.
>    *
>    * @param aP
>    *        The parent DOM node.
>    * @param aUnsafePrePath
>    *        The partial unsafePath leading up to this node.
>    * @param aT
>    *        The tree.
>+   * @param aBaseIndentText
>+   *        The base text of the indent, which may be augmented within the
>+   *        functton.

functton

>+      for (var i = 0; i < aT._kids.length; i++) {
>+        // 3 is the standard depth, the callee adjusts it if necessary.
>+        aIndentGuide.push({ _isLastKid: (i === aT._kids.length - 1), _depth: 3 });
>+        // Generate the base indent.
>+        var baseIndentText = "";
>+        if (aIndentGuide.length > 0) {
>+          for (var j = 0; j < aIndentGuide.length - 1; j++) {
>+            baseIndentText += aIndentGuide[j]._isLastKid ? " " : kVertical;
>+            baseIndentText += repeatStr(" ", aIndentGuide[j]._depth - 1);
>+          }
>+          baseIndentText += aIndentGuide[j]._isLastKid ?
>+                            kUpAndRight : kVerticalAndRight;
>+          baseIndentText += repeatStr(kHorizontal, aIndentGuide[j]._depth - 1);
>+        }
>+
>+        appendTreeElements2(d, unsafePath + "/", aT._kids[i], aIndentGuide,
>+                            baseIndentText, tString.length);
>+        aIndentGuide.pop();
>+      }

Why is baseIndentText not created with an array?
Comment 26 Justin Lebar (not reading bugmail) 2012-02-09 13:51:04 PST
Comment on attachment 594902 [details] [diff] [review]
repeatStr

Oh, baseIndentText is now an array here.  Cool.
Comment 29 neil@parkwaycc.co.uk 2012-02-15 03:50:27 PST
(In reply to Justin Lebar from comment #9)
> Looks like you really want string.startsWith, which we don't have.  So
> you're stuck with indexOf == 0, which is probably fine
this._unsafePath.lastIndexOf(aTreeName, 0) === 0
[since lastIndexOf works backwards to 0, this only does one comparison]
Comment 30 Nicholas Nethercote [:njn] 2012-02-15 16:46:38 PST
> this._unsafePath.lastIndexOf(aTreeName, 0) === 0

Nice, I'll try to remember that for the future.  But this shouldn't affect the memory consumption, which is what this bug is about.
Comment 31 neil@parkwaycc.co.uk 2012-02-16 02:40:36 PST
(In reply to Nicholas Nethercote from comment #30)
> > this._unsafePath.lastIndexOf(aTreeName, 0) === 0
> Nice, I'll try to remember that for the future.  But this shouldn't affect
> the memory consumption, which is what this bug is about.
Don't worry, I'm not going to go filing bugs on everyone that wants startsWith.

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