674158 - OOify aboutMemory.js

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

aboutMemory.js is written in a very imperative style.  This makes it hard to
read, particularly because there are three similar but different object
types used to represent different kinds of memory reporters.

This patch makes aboutMemory.js more object-oriented, which makes the object
types clearer.  This is preliminary work that will allow aboutMemory.js to
be made simpler and consume less memory (which perturbs the measurements).

This patch also increases the amount of sanity checking done, asserting
(throwing exceptions) in more cases.

Comment 1

[Nicholas Nethercote [inactive]]

dolske: any idea when you'll have a chance to review?

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 548365 [details] [diff] [review]
patch

Review of attachment 548365 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +156,5 @@
> +      this._amount += r._amount;
> +    } else if (this._amount === kUnknown && r._amount !== kUnknown) {
> +      this._amount = r._amount;
> +    }
> +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;

Can r._nMerged ever be nonzero?

@@ +292,5 @@
> +
> +TreeNode.prototype = {
> +  findKid: function(aName) {
> +    for (var i = 0; i < this._kids.length; i++) {
> +      if (this._kids[i]._name === aName) {

I guess this is just rectoring, but why strict-equality here?

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to Justin Dolske [:Dolske] from comment #2)
> ::: toolkit/components/aboutmemory/content/aboutMemory.js
> @@ +156,5 @@
> > +      this._amount += r._amount;
> > +    } else if (this._amount === kUnknown && r._amount !== kUnknown) {
> > +      this._amount = r._amount;
> > +    }
> > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> 
> Can r._nMerged ever be nonzero?

Yes, if there are 3 or more reporters with the same path.


> @@ +292,5 @@
> > +
> > +TreeNode.prototype = {
> > +  findKid: function(aName) {
> > +    for (var i = 0; i < this._kids.length; i++) {
> > +      if (this._kids[i]._name === aName) {
> 
> I guess this is just rectoring, but why strict-equality here?

I use it everywhere because Crockford recommends it and I think he's right on this one.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to Nicholas Nethercote [:njn] from comment #3)

> > > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> > 
> > Can r._nMerged ever be nonzero?
> 
> Yes, if there are 3 or more reporters with the same path.

Hmm, this shouldn't this code take that into account? It's only looking at this._nMerged, not r._nMerged.

Maybe just call it ._numReporters (or whatever), have each init to 1 by default, and then blindly add them together here?

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Justin Dolske [:Dolske] from comment #4)
> (In reply to Nicholas Nethercote [:njn] from comment #3)
> 
> > > > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> > > 
> > > Can r._nMerged ever be nonzero?
> > 
> > Yes, if there are 3 or more reporters with the same path.
> 
> Hmm, this shouldn't this code take that into account? It's only looking at
> this._nMerged, not r._nMerged.

Oh, I see.  Sorry, r._nMerged cannot be non-zero.  I'll add an assertion to that effect.

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/48c58d9cc3f3

Comment 7

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/48c58d9cc3f3