722972 - Reduce about:memory perturbation

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

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 [inactive]]

Attachment: cleanups

This first patch doesn't affect memory allocation, it just removes some dead variables and parameters.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: 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 [inactive]]

Attachment: 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 [inactive]]

Attachment: hasKids-only

This patch avoids doing some unnecessary work when |hasKids| is false.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: redo-indent

This patch avoids generating identical indent strings for each child of a node.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: 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 [inactive]]

Attachment: formatInt

Use Array.join() in formatInt().  Also include the unit in the join() when appropriate.

Comment 9

[Justin Lebar (not reading bugmail)]

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)]

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)]

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)]

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)]

(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)]

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)]

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)]

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 [inactive]]

Attachment: 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 [inactive]]

> 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 [inactive]]

> 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 [inactive]]

> >+// 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 [inactive]]

> 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 [inactive]]

Attachment: formatInt v2

This version adds the requested comment.

Comment 23

[Justin Lebar (not reading bugmail)]

> 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)]

> > 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)]

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)]

Comment on attachment 594902 [details] [diff] [review]
repeatStr

Oh, baseIndentText is now an array here.  Cool.

Comment 27

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3afa5cd75ff2
https://hg.mozilla.org/integration/mozilla-inbound/rev/28a73ec71520
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac5c3ea6116
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fc70ceee8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/59350a8c846d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d87c5262c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/00003cb750ff

Thanks for all the reviews, jlebar!

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3afa5cd75ff2
https://hg.mozilla.org/mozilla-central/rev/28a73ec71520
https://hg.mozilla.org/mozilla-central/rev/0ac5c3ea6116
https://hg.mozilla.org/mozilla-central/rev/c7fc70ceee8f
https://hg.mozilla.org/mozilla-central/rev/59350a8c846d
https://hg.mozilla.org/mozilla-central/rev/a1d87c5262c1
https://hg.mozilla.org/mozilla-central/rev/00003cb750ff

Comment 29

[[e-mail]]

(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 [inactive]]

> 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

[[e-mail]]

(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.