Reduce about:memory perturbation

RESOLVED FIXED in mozilla13

Status

()

Toolkit
about:memory
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Depends on: 1 bug)

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(7 attachments, 2 obsolete attachments)

1.33 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
5.66 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
6.36 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
5.08 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
2.91 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
1.02 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
2.22 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
Depends on: 724479
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
Created attachment 594895 [details] [diff] [review]
cleanups

This first patch doesn't affect memory allocation, it just removes some dead variables and parameters.
Attachment #594895 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 3

6 years ago
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.
Attachment #594896 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 4

6 years ago
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.
Attachment #594897 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 5

6 years ago
Created attachment 594898 [details] [diff] [review]
hasKids-only

This patch avoids doing some unnecessary work when |hasKids| is false.
Attachment #594898 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 6

6 years ago
Created attachment 594901 [details] [diff] [review]
redo-indent

This patch avoids generating identical indent strings for each child of a node.
Attachment #594901 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 7

6 years ago
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.
Attachment #594902 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 8

6 years ago
Created attachment 594903 [details] [diff] [review]
formatInt

Use Array.join() in formatInt().  Also include the unit in the join() when appropriate.
Attachment #594903 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

6 years ago
Attachment #594898 - Attachment is patch: true
Attachment #594895 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #594896 - Flags: review?(justin.lebar+bug) → review-
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 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.)
Attachment #594897 - Flags: review?(justin.lebar+bug) → review+
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?
Attachment #594898 - Flags: review?(justin.lebar+bug) → review+
(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?
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 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?
Attachment #594903 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #594901 - Flags: review?(justin.lebar+bug) → review-
Attachment #594902 - Flags: review?(justin.lebar+bug) → review-
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 17

6 years ago
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.
Attachment #594896 - Attachment is obsolete: true
Attachment #595314 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 18

6 years ago
> 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.
(Assignee)

Comment 19

6 years ago
> 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.
(Assignee)

Comment 20

6 years ago
> >+// 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.
(Assignee)

Comment 21

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Attachment #594902 - Flags: review- → review?
(Assignee)

Updated

6 years ago
Attachment #594901 - Flags: review- → review?
(Assignee)

Comment 22

6 years ago
Created attachment 595317 [details] [diff] [review]
formatInt v2

This version adds the requested comment.
Attachment #594903 - Attachment is obsolete: true
Attachment #595317 - Flags: review?(justin.lebar+bug)
> 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?
Attachment #595314 - Flags: review?(justin.lebar+bug) → review+
Attachment #595317 - Flags: review?(justin.lebar+bug) → review+
> > 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.
(Assignee)

Updated

6 years ago
Attachment #594901 - Flags: review? → review?(justin.lebar+bug)
(Assignee)

Updated

6 years ago
Attachment #594902 - Flags: review? → review?(justin.lebar+bug)
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?
Attachment #594901 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 594902 [details] [diff] [review]
repeatStr

Oh, baseIndentText is now an array here.  Cool.
Attachment #594902 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 27

6 years ago
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!
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(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]
(Assignee)

Comment 30

5 years ago
> 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.
(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.
You need to log in before you can comment on or make changes to this bug.