723799 - Use mallocSizeOf in the DOM memory reporters

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nicholas Nethercote [inactive]]

We need to convert the existing DOM reporters to the new style.  See
https://wiki.mozilla.org/Memory_Reporting#A_Simple_Example for details of 
what this style looks like.  Short version of the advantages:

- When measuring an object, it's always clear if you are measuring/should 
  measure the object itself or just the things hanging off it.

- Measures slop bytes.
  
- No error-prone size computations.

- Provides integration with DMD, which provides detection of unreported and
  double-reported blocks.

Patch coming shortly.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch

There's nothing terribly surprising here, once you're used to the new style of reporter.

Comment 3

[Mounir Lamouri (:mounir)]

This review is pretty big and is about stuff I don't know very well. I will try to have it done this week though. Feel free to ping me next week if I give no update here.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> This review is pretty big and is about stuff I don't know very well.

Didn't you write the existing DOM memory reporters?  All I've done is modify them.  The new style will be unfamiliar but https://wiki.mozilla.org/Memory_Reporting#A_Simple_Example explains it.

Comment 5

[Mounir Lamouri (:mounir)]

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

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

I still don't feel highly comfortable: even if I wrote most of what you changed, writing it doesn't mean I'm able to understand all possible issues that might happen when a change is done.

Anyhow, r=me.

::: content/base/public/nsIDocument.h
@@ +1619,5 @@
>  
> +  // Note: nsIDocument is a sub-class of nsINode, which has a
> +  // SizeOfExcludingThis function.  However, because nsIDocument objects can
> +  // only appear at the top of the DOM tree, we have a specialized measurement
> +  // function which returns multiple sizes.

See my comment about this below.

::: content/base/src/nsDocument.cpp
@@ +9162,5 @@
> +                                     aWindowSizes->mMallocSizeOf); 
> +  aWindowSizes->mDOM +=
> +    mAttrStyleSheet ?
> +    mAttrStyleSheet->DOMSizeOfIncludingThis(aWindowSizes->mMallocSizeOf) :
> +    0;

nit: that might be nicer to write this like:
aWindowSizes->mDOM += mAttrStyleSheet
                        ? mAttrStyleSheet->DOMSizeOfIncludingThis(aWindowSizes->mMallocSizeOf)
                        : 0;

::: content/base/src/nsDocument.h
@@ +996,5 @@
>  
> +  virtual NS_MUST_OVERRIDE void
> +    DocSizeOfExcludingThis(nsWindowSizes* aWindowSizes) const;
> +  virtual void
> +    DocSizeOfIncludingThis(nsWindowSizes* aWindowSizes) const = 0;

This design is a bit disturbing (keeping SizeOfIncludingThis and SizeOfExcludingThis but not expecting those to be called and adding new method prefixed by |Doc|). I wonder if that would be interesting to at least keep the same name and just have the method to differ because of the signature.

Also, I think you should not make DocSizeOfIncludingThis virtual pure for the reasons I will give below.

::: content/base/src/nsTextFragment.cpp
@@ +429,5 @@
>  
>  }
>  
> +/* virtual */ size_t
> +nsTextFragment::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const

This method seems a bit different from what it was previously doing.
Could you explain why?

::: content/base/src/nsTextFragment.h
@@ +222,5 @@
>      PRUint32 mIsBidi : 1;
>      PRUint32 mLength : 29;
>    };
>  
> +  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;

The |SizeOf| method is returning the size of the object including this. Why do you use |SizeOfExcludingThis|? Have callers changed?

::: content/html/content/src/nsGenericHTMLElement.h
@@ +82,5 @@
>                   "Unexpected namespace");
>    }
>  
> +  NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS(nsGenericHTMLElementBase)
> +  NS_DEFINE_SIZEOF_INCLUDING_THIS

Most classes inheriting from nsGenericHTMLElement doesn't implement SizeOfIncludingThis and SizeOfExcludingThis, thus will generate a static analysis error because of the NS_MUST_OVERRIDE.

@@ +878,5 @@
> +  // worthwhile:
> +  // - mForm
> +  // - mFieldSet
> +  NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS(nsGenericHTMLElement)
> +  NS_DEFINE_SIZEOF_INCLUDING_THIS

ditto

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +279,5 @@
> +  // worthwhile:
> +  // - mFrameLoader (bug 672539)
> +  // - mTitleChangedListener
> +
> +  return n;

nit: you could simply:
|return nsGenericHTMLElement::SizeOfExcludingThis(aMallocSizeOf);

::: content/html/document/src/nsHTMLDocument.cpp
@@ +3472,5 @@
> +/* virtual */ void
> +nsHTMLDocument::DocSizeOfIncludingThis(nsWindowSizes* aWindowSizes) const
> +{
> +  aWindowSizes->mDOM += aWindowSizes->mMallocSizeOf(this);
> +  DocSizeOfExcludingThis(aWindowSizes);

ditto: maybe that could be directly defined in nsDocument?

::: content/xml/document/src/nsXMLDocument.cpp
@@ +604,5 @@
> +/* virtual */ void
> +nsXMLDocument::DocSizeOfIncludingThis(nsWindowSizes* aWindowSizes) const
> +{
> +  aWindowSizes->mDOM += aWindowSizes->mMallocSizeOf(this);
> +  DocSizeOfExcludingThis(aWindowSizes);

It seems that all |DocSizeOfIncludingThis| implementation are doing exactly this. If that's the case, may be you could just make nsDocument defining this method that way?

::: layout/base/nsPresShell.cpp
@@ -687,5 @@
>  
>    return PL_DHASH_NEXT;
>  }
>  
> -NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(GfxTextrunWordCacheMallocSizeOf, "gfx/textrun-word-cache")

How is that related to this bug?

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: additional patch

Thanks for the nice review, Mounir.  I've changed enough things while
addressing your comments that I'm asking for another review.  The attached
patch applies on top of the original patch.


> nit: that might be nicer to write this like:
> aWindowSizes->mDOM += mAttrStyleSheet
>                         ? mAttrStyleSheet->DOMSizeOfIncludingThis(aWindowSizes
->mMallocSizeOf)
>                         : 0;

But then it exceeds 80 characters! :)


> This design is a bit disturbing (keeping SizeOfIncludingThis and
> SizeOfExcludingThis but not expecting those to be called and adding new
> method prefixed by |Doc|). I wonder if that would be interesting to at
> least keep the same name and just have the method to differ because of the
> signature.

At one point I was going to drop the "Doc" prefix but then I decided I liked
having it because it made it easier to find these special cases in the code.
Sometimes overloading is nice, but sometimes it makes things more confusing
and I felt this was one of those latter cases.  Sound reasonable?


> > +/* virtual */ size_t
> > +nsTextFragment::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>
> This method seems a bit different from what it was previously doing.
> Could you explain why?

The old version was wrong.  If Is2b() is false, |m1b| can point to a static
(i.e. non-heap) buffer, |sSingleCharSharedString|.  That shouldn't be
measured by the memory reporter, and indeed, calling aMallocSizeOf() on it
causes crashes.


> > +  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
>
> The |SizeOf| method is returning the size of the object including this.
> Why do you use |SizeOfExcludingThis|? Have callers changed?

The callsite (prior to my patch) looks like this:

  PRInt64
  nsGenericDOMDataNode::SizeOf() const
  {
    PRInt64 size = dom::MemoryReporter::GetBasicSize<nsGenericDOMDataNode,
                                                     nsIContent>(this);
    size += mText.SizeOf() - sizeof(mText);
    return size;
  }

You were having to subtract |sizeof(mText)|.  With my patch it looks like this:

  size_t
  nsGenericDOMDataNode::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) con
st
  {
    size_t n = nsIContent::SizeOfExcludingThis(aMallocSizeOf);
    n += mText.SizeOfExcludingThis(aMallocSizeOf);
    return n;
  }

That's one of the nice things about having the IncludingThis/ExcludingThis
distinction, this kind of thing is much clearer :)


> Most classes inheriting from nsGenericHTMLElement doesn't implement
> SizeOfIncludingThis and SizeOfExcludingThis, thus will generate a static
> analysis error because of the NS_MUST_OVERRIDE.
>
> @@ +878,5 @@
> > +  // worthwhile:
> > +  // - mForm
> > +  // - mFieldSet
> > +  NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS(nsGenericHTMLElement)
> > +  NS_DEFINE_SIZEOF_INCLUDING_THIS
>
> ditto

Good catch.  Goodness, there are a lot of them.  I've added definitions for
all of them, and noted which ones have pointers to other things that may be
worth measuring in the future.


> nit: you could simply:
> |return nsGenericHTMLElement::SizeOfExcludingThis(aMallocSizeOf);

Done.
 
                                 
> ::: content/xml/document/src/nsXMLDocument.cpp
> @@ +604,5 @@
> > +/* virtual */ void
> > +nsXMLDocument::DocSizeOfIncludingThis(nsWindowSizes* aWindowSizes) const
> > +{
> > +  aWindowSizes->mDOM += aWindowSizes->mMallocSizeOf(this);
> > +  DocSizeOfExcludingThis(aWindowSizes);
>
> It seems that all |DocSizeOfIncludingThis| implementation are doing
> exactly this. If that's the case, may be you could just make nsDocument
> defining this method that way?

Hmm, good point.  Actually, it's even better to move it into nsIDocument.
I've done that.

Actually, the same thing is true for all the sub-classes beneath nsINode --
NS_DEFINE_SIZEOF_INCLUDING_THIS isn't necessary.  I've removed all them too.
And likewise for nsIJSEventListener.

And I've updated
https://wiki.mozilla.org/Platform/Memory_Reporting#An_Example_Involving_Inherita
nce.
Thanks for spotting this!  It's a good review that gives you a better
understanding of your own code :)


> > -NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(GfxTextrunWordCacheMallocSizeOf, "gfx/
textrun-word-cache")
>
> How is that related to this bug?

It's a dead declaration and I found it while working on this patch, so I
decided to remove it.

Comment 7

[Justin Lebar (not reading bugmail)]

> Most classes inheriting from nsGenericHTMLElement doesn't implement
> SizeOfIncludingThis and SizeOfExcludingThis, thus will generate a static
> analysis error because of the NS_MUST_OVERRIDE.

I don't get why NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS is ever necessary.  Isn't the point of virtual functions that you don't need to do this?

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> > Most classes inheriting from nsGenericHTMLElement doesn't implement
> > SizeOfIncludingThis and SizeOfExcludingThis, thus will generate a static
> > analysis error because of the NS_MUST_OVERRIDE.
> 
> I don't get why NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS is ever
> necessary.  Isn't the point of virtual functions that you don't need to do
> this?

Precisely because of the NS_MUST_OVERRIDE.  If you add SizeOf{In,Ex}cludingThis to class A, it's really easy to forget to do likewise to sub-classes of A.  If those sub-classes don't contain any members that point to other objects, this won't matter.  But if those functions are missing it's not obvious whether the class doesn't need them or if they were forgotten.  By adding the NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS it makes it clear it's the former.  So:  explicit is better than implicit.

Having said that, there are lots of sub-classes of nsGenericHTMLElement, most of them don't have members that point to other objects, and I'm not totally convinced by my argument in the above paragraph :/

Comment 9

[Justin Lebar (not reading bugmail)]

> So:  explicit is better than implicit.

I agree with this, of course.

But NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS is only *implicitly* saying "I choose not to measure the members in this class."  If you want to say something about it, wouldn't a comment saying we use the inherited version be better?

Worse than the comment but better than the current approach, I think, would be explicitly defining NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS to the empty string.  At least then it's clear what it does...

> Precisely because of the NS_MUST_OVERRIDE.

I don't understand.  NS_MUST_OVERRIDE means "this function must override some other function."  It does not mean "this function must be overridden somewhere."

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> > So:  explicit is better than implicit.
> 
> I agree with this, of course.
> 
> But NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS is only *implicitly*
> saying "I choose not to measure the members in this class."  If you want to
> say something about it, wouldn't a comment saying we use the inherited
> version be better?

In cases where there are pointer members that could be measured but are not measured, I've been writing comments like this:

   // Measurement of the following members may be added later if DMD finds it is
   // worthwhile:
   // - mForm
   // - mFieldSet
   NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS(nsGenericHTMLElement)

In cases where the are *no* pointer members to measure I've just been using the 
NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS without a comment.  I guess I could add a comment like "At the time of writing, this sub-class has no data members that need to be measured."

> Worse than the comment but better than the current approach, I think, would
> be explicitly defining NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS to
> the empty string.  At least then it's clear what it does...

Maybe... now I'm totally undecided about what's best :(


> I don't understand.  NS_MUST_OVERRIDE means "this function must override
> some other function."  It does not mean "this function must be overridden
> somewhere."

Yesterday was a day where I discovered several things I thought to be true were not... but I believe I'm right on this one! :)  From nscore.h:

 * NS_MUST_OVERRIDE:
 *   a method which every immediate subclass of this class must
 *   override.  A subclass override can itself be NS_MUST_OVERRIDE, in
 *   which case its own subclasses must override the method as well.
 *
 *   This is similar to, but not the same as, marking a method pure
 *   virtual.  It has no effect on the class in which the annotation
 *   appears, you can still provide a definition for the method, and
 *   it objects to the mere existence of a subclass that doesn't
 *   override the method.  See examples in analysis/must-override.js.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 595956 [details] [diff] [review]
additional patch

I have some tweaks to make to this patch, I'll update it again shortly.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: additional patch, v2

Updated additional patch, still applies on top of the first patch.  Changes:

- For all sub-classes of nsINode that defined a SizeOfExcludingThis that 
  didn't do anything different to the super-class, I just removed the 
  SizeOfExcludingThis definition and let them inherit from the super-class.  

  Unmeasured fields in sub-classes are tracked in the comment above
  nsINode::SizeOfExcludingThis.
  
- I clarified why inheriting SizeOfIncludingThis is safe for sub-classes of
  nsINode.  (It's not safe in general.)
  
- I removed the NS_MUST_OVERRIDE annotations, I'm not going to use them any
  more.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: combined patch

Here's a patch combining "patch" and "additional patch, v2", in case that's easier to read.  jlebar, take a look if you want;  the nsINode.h changes are probably most interesting to you.

Comment 14

[Justin Lebar (not reading bugmail)]

> +  // Among the sub-classes that inherit (directly or indirectly) from nsINode,
> +  // measurement of the following members may be added later if DMD finds it is
> +  // worthwhile:

I'm not sure this comment is useful -- how will we ensure it's kept up to date if new members are added or removed from these subclasses?  Better would be a list of classes which have members and don't implement SizeOfExcludingThis.  But even that can and will go out of date, and if it's not complete, what's the point?

If a comment should be anywhere, I'd put it right by the members of the relevant classes.

> +  // This SizeOfExcludingThis() is the one inherited from nsINode.  But
> +  // nsDocuments can only appear at the top of the DOM tree, and we use the
> +  // specialized DocSizeOfExcludingThis() in that case.  So this should never
> +  // be called.
> +  NS_ABORT_IF_FALSE(false, "nsDocument::SizeOfExcludingThis called");

"This SizeOfExcludingThis() implementation *overrides* the one from nsINode"?

Also, you could use NS_ABORT or MOZ_NOT_REACHED.

Comment 15

[Justin Lebar (not reading bugmail)]

I filed bug 727279 on renaming NS_MUST_OVERRIDE to NS_MUST_BE_OVERRIDDEN, since I was completely confused by what it meant.

Comment 16

[Mounir Lamouri (:mounir)]

I will try to get to this review by the end of next week. I was in a work week this week and will be in another next week, that explains the delays.

Comment 17

[Nicholas Nethercote [inactive]]

Mounir, do you think you'll get to this any time soon?  I have r+ from you on the first patch and f+ from jlebar on the combined patch, I'm tempted to just declare victory and land it.

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 597254 [details] [diff] [review]
additional patch, v2

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

r=me

Sorry for the delay, I had two work weeks in a row and I got sick in the middle of that... :-/

::: content/html/content/src/nsHTMLAreaElement.cpp
@@ +61,5 @@
>    // nsISupports
>    NS_DECL_ISUPPORTS_INHERITED
>  
> +  // TODO: nsHTMLAnchorElement::SizeOfAnchorElement should call
> +  // Link::SizeOfExcludingThis().  See bug 682431.

Why is this comment in nsHTMLAreaElement.cpp?

::: content/html/content/src/nsHTMLLinkElement.cpp
@@ +85,5 @@
>    // nsIDOMHTMLLinkElement
>    NS_DECL_NSIDOMHTMLLINKELEMENT
>  
> +  // TODO: nsHTMLAnchorElement::SizeOfAnchorElement should call
> +  // Link::SizeOfExcludingThis().  See bug 682431.

ditto

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c30b6943dc98

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/c30b6943dc98