Together these account for 4,480 B on OS X in static SVG-as-an-image documents where there is no user interaction.

Comment on attachment 8429630 [details] [diff] [review] PresShell::mSelection Review of attachment 8429630 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsSelection.cpp @@ +2956,5 @@ > + for (int32_t i = 0; i < nsISelectionController::NUM_SELECTIONTYPES; ++i){ > + if (mDomSelections[i]) { > + // TODO: This doesn't include the size of objects that each Selection > + // points to. > + total += aMallocSizeOf(mDomSelections[i]); This should be > total += mDomSelections[i]->SizeOfIncludingThis(aMallocSizeOf); which will require implementing Selection::SizeOfIncludingThis().

Comment on attachment 8429631 [details] [diff] [review] PresShell::mCaret Review of attachment 8429631 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCaret.cpp @@ +872,5 @@ > +{ > + size_t total = aMallocSizeOf(this); > + if (mPresShell) { > + // This is the size of the nsWeakReference object, not the pres shell > + total += aMallocSizeOf(mPresShell.get()); This looks strange and makes me nervous. It would be better if nsWeakPtr has a SizeOfExcludingThis() function and you called that. Ditto for the cases below.

Comment on attachment 8429634 [details] [diff] [review] PresShell::mFrameConstructor Review of attachment 8429634 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCounterManager.cpp @@ +305,5 @@ > + mozilla::MallocSizeOf mallocSizeOf, > + void* userArg) > +{ > + // TODO: Maybe count the members of aData. > + return mallocSizeOf(aData.get()) + Again, the direct mallocSizeOf() call is bad. Nit: should be |aMallocSizeOf| and |aUserArg|.

I've looked at the patches and posted my comments. A layout expert should look at them too, though.

(In reply to Nicholas Nethercote [:njn] from comment #7) > This looks strange and makes me nervous. It would be better if nsWeakPtr has > a SizeOfExcludingThis() function and you called that. Ditto for the cases > below. Given that nsWeakPtr is just a typedef (which I tend to think we should get rid of and write out at the callers): typedef nsCOMPtr<nsIWeakReference> nsWeakPtr; I think this should probably be a SizeOfIncludingThis method (on nsIWeakReference, I suppose). Do we have a convention of modifying relatively low-level interfaces like this to add SizeOf methods? And does that make sense to you?

> Do we have a convention of modifying > relatively low-level interfaces like this to add SizeOf methods? And does > that make sense to you? It does make sense. We already do this for a few cases; see xpcom/base/nsISizeOf.h and dom/base/nsISizeOfEventTarget.h.

Nicholas: so I guess what you're saying is, only call aMallocSizeOf on |this|, in general? (Rather than saying go down the rabbit hole and track down everything along the chains you touch to implement SizeOfIncludingThis() calls as far along the chain as things go.)

(In reply to Nicholas Nethercote [:njn] from comment #7) > Comment on attachment 8429631 [details] [diff] [review] > PresShell::mCaret > > Review of attachment 8429631 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsCaret.cpp > @@ +872,5 @@ > > +{ > > + size_t total = aMallocSizeOf(this); > > + if (mPresShell) { > > + // This is the size of the nsWeakReference object, not the pres shell > > + total += aMallocSizeOf(mPresShell.get()); > > This looks strange and makes me nervous. It would be better if nsWeakPtr has > a SizeOfExcludingThis() function and you called that. Well, SizeOfOnlyThis, but sure.

(In reply to Nicholas Nethercote [:njn] from comment #11) > It does make sense. We already do this for a few cases; see > xpcom/base/nsISizeOf.h and dom/base/nsISizeOfEventTarget.h. I don't think it makes sense to take the cost of the nsISizeOf interface though, at least in this case. This can be done in a more lightweight fashion by using |%{C++ ... %}| blocks in the IDL.

Comment on attachment 8430053 [details] [diff] [review] RuleCascadeData::mKeyframesRuleTable >+static size_t >+SizeOfKeyframesRuleEntryExcludingThis(nsStringHashKey::KeyType& aKey, >+ nsCSSKeyframesRule* const& aData, >+ mozilla::MallocSizeOf aMallocSizeOf, >+ void* aUserArg) >+{ >+ // We don't own the nsCSSKeyframesRule objects so we don't count them. We do >+ // care about the size of the keys' nsString members' buffers though. >+ return aKey.SizeOfExcludingThisIfUnshared(aMallocSizeOf); Maybe s/nsString/nsAString/ in the comment? (since nsStringHashKey::KeyType is nsAString&, and I don't think there's necessarily any nsString instances involved here.) ALSO: I don't think the parameter "nsStringHashKey::KeyType&" should technically have an ampersand. I'm pretty sure this function needs to match the signature here: 225 typedef size_t 226 (* SizeOfEntryExcludingThisFun)(KeyType aKey, 227 const DataType &aData, 228 mozilla::MallocSizeOf mallocSizeOf, 229 void* userArg); http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsBaseHashtable.h#216 Note the "KeyType" (not KeyType&) in that signature. (Under-the-hood, your KeyType is itself a reference to a nsAString, but that shouldn't influence whether we use KeyType vs. KeyType&) r=me with the above addressed

(In reply to Jonathan Watt [:jwatt] from comment #12) > Nicholas: so I guess what you're saying is, only call aMallocSizeOf on > |this|, in general? Yes. (There are some rare exceptions for really trivial structs that have no methods and no pointers.) > (Rather than saying go down the rabbit hole and track > down everything along the chains you touch to implement > SizeOfIncludingThis() calls as far along the chain as things go.) Well, sometimes that's the right thing to do! But it depends on how big the things are. That's why DMD is useful, because it can give you data on which fields are unimportant. And that explains why there are lots of comments saying "we only measure some members, and we'll measure more if DMD says it's necessary".

Comment on attachment 8429976 [details] [diff] [review] PresShell::mSelection Review of attachment 8429976 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsSelection.cpp @@ +112,5 @@ > , mCanCacheFrameOffset(false) > {} > > + size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { > + return aMallocSizeOf(this); It'd be good to have a comment here explaining why you haven't measured the members (as you've done below). @@ +225,5 @@ > return NS_OK; > } > + > + size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { > + return aMallocSizeOf(this); Ditto.

Comment on attachment 8430054 [details] [diff] [review] PresShell::mFrameConstructor Review of attachment 8430054 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCounterManager.cpp @@ +189,5 @@ > +{ > + size_t total = aMallocSizeOf(this); > + // TODO: This doesn't account for memory that our base class has owning > + // pointers to. > + return total; Ideally you'd add SizeOf*() methods to the base class and then invoke that as described in https://wiki.mozilla.org/Memory_Reporting#An_Example_Involving_Inheritance, though maybe those base class members aren't significant enough to bother.

Comment on attachment 8430054 [details] [diff] [review] PresShell::mFrameConstructor >diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp >+size_t >+nsCSSFrameConstructor::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const >+{ >+ size_t total = aMallocSizeOf(this); >+ >+ total += mCounterManager.SizeOfExcludingThis(aMallocSizeOf); >+ >+ return total; >+} Are there any other members on nsCSSFrameConstructor whose memory we need to report here? Looks like at least mQuoteList (ExcludingThis) and mTempFrameTreeState (IncludingThis, if it's non-null). Also, there are a few things that are inherited from nsFrameManagerBase (via nsFrameManager, nsCSSFrameConstructor's superclass) -- I suspect we need to account for mUndisplayedMap (a pointer): http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManagerBase.h#67 and possibly mPlaceholderMap (a PLDHashTable)? (not sure if aMallocSizeOf(this) correctly-handles PLDHashTable members. I don't think we need to worry about enumerating the hash tables' contents, since it just contains pointers to frames in the frame tree, but we do need to be sure the table itself is being correctly reported) If you'd like to defer those to a followup, that's OK with me, as long as it's tracked in a bug and mentioned in a comment here (with a bug number, ideally). Otherwise, it looks like this function is complete, when really there's more stuff we should be measuring. >+++ b/layout/base/nsCounterManager.cpp >+size_t >+nsCounterList::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const >+{ >+ size_t total = aMallocSizeOf(this); >+ // TODO: This doesn't account for memory that our base class has owning >+ // pointers to. >+ return total; >+} The base class is where the actual list itself is stored, right? (nsGenConList has a "nsGenConNode* mFirstNode;", which presumably is what the rest of the nodes in the list are chained off of) This means, in response to comment 22, I don't think the base class members are insignificant enough to be ignored here. I think we do need memory-reporting for nsGenConList/nsGenConNode. If you'd like to defer that to a followup, please reference a bug number in this comment. (and in a similar comment in nsQuoteList, if/when you add memory-reporting for mQuoteList, per my previous comment, since that also derives from nsGenConList) >+static size_t >+SizeOfNamesEntryExcludingThis(nsStringHashKey::KeyType& aKey, >+ const nsAutoPtr<nsCounterList>& aData, >+ mozilla::MallocSizeOf aMallocSizeOf, >+ void* userArg) >+{ >+ MOZ_ASSERT(aData); Could you add a brief assertion-message? e.g. "unexpected null pointer in mNames hash table". (If this ever fails, that'll make for a somewhat-less-inscrutable abort, and will make it easier for a bug report to be filed with a searchable summary.) >diff --git a/layout/base/nsCounterManager.h b/layout/base/nsCounterManager.h >+ virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; >+ Presumably this is "virtual" because you're intending to add a nsGenConList impl, which this will override. Maybe add... // TODO: Mark this MOZ_OVERRIDE when bug {nsGenConList-bug-number} is fixed above this, so that we don't forget? (and fill in {nsGenConList-bug-number} with whatever bug you file for that)

> not sure if aMallocSizeOf(this) correctly-handles PLDHashTable members. Please use PL_DHashTableSizeOf{In,Ex}cludingThis() to measure PLDHashTables!

(In reply to Daniel Holbert [:dholbert] from comment #19) > Note the "KeyType" (not KeyType&) in that signature. (Under-the-hood, your > KeyType is itself a reference to a nsAString, but that shouldn't influence > whether we use KeyType vs. KeyType&) Hmm, it compiled. I've changed it though, and added a comment noting that we depend on nsStringHashKey::GetKey() returning a reference to the key in order to be measuring the correct object.

(In reply to Nicholas Nethercote [:njn] from comment #20) > (In reply to Jonathan Watt [:jwatt] from comment #12) > > (Rather than saying go down the rabbit hole and track > > down everything along the chains you touch to implement > > SizeOfIncludingThis() calls as far along the chain as things go.) > > Well, sometimes that's the right thing to do! But it depends on how big the > things are. That's why DMD is useful, because it can give you data on which > fields are unimportant. And that explains why there are lots of comments > saying "we only measure some members, and we'll measure more if DMD says > it's necessary". I'm working on this based off of DMD results, prioritizing the stuff that's showing up as significant.

Comment on attachment 8430054 [details] [diff] [review] PresShell::mFrameConstructor Marking r- on the PresShell::mFrameConstructor patch, to make per comment 23 explicit & remove this from my review queue. :) (This is close, though, I think.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f64945e5c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a38c2b23536

https://hg.mozilla.org/mozilla-central/rev/a9f64945e5c6 https://hg.mozilla.org/mozilla-central/rev/9a38c2b23536

Comment on attachment 8438484 [details] [diff] [review] PresShell::mFrameConstructor and all the stuff it owns >-struct nsCounterUseNode : public nsCounterNode { >+struct nsCounterUseNode MOZ_FINAL : public nsCounterNode { > // The same structure passed through the style system: an array > // containing the values in the counter() or counters() in the order > // given in the CSS spec. > nsRefPtr<nsCSSValue::Array> mCounterStyle; [...] >+ virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE { >+ size_t total = 0; >+ total += nsCounterNode::SizeOfExcludingThis(aMallocSizeOf); >+ if (mCounterStyle) { >+ total += mCounterStyle->SizeOfIncludingThis(aMallocSizeOf); mCounterStyle is a nsRefPtr, so we aren't necessarily the sole owner. Do we know that this is the right place to be reporting its memory from? (and that no one else will also be reporting its memory?) From a quick skim, it looks like its value comes from nsCSSFrameConstructor::CreateGeneratedContent(), and there, we get it from aStyleContext->StyleContent(). So maybe this memory is already owned by (and reported by) the style system? Either way, please add a comment here explaining why this is/isn't the right place to report mCounterStyle's memory. >-class nsCounterList : public nsGenConList { >+class nsCounterList MOZ_FINAL : public nsGenConList { [...] >+ // SizeOfExcludingThis is not implemented. We don't have anything to add >+ // and, since our sub-classes implement it, we'd have to make it virtual. >+ // That would bloat each instances of this class with a vtable pointer >+ // which we want to avoid. Not sure what "our sub-classes" refers to here; nsCounterList doesn't have any subclasses. (and you're making it MOZ_FINAL) I think you want s/sub-classes implement/base-class implements/? And probably "s/not implemented/inherited/". ("not implemented" sounds like this class doesn't have an implementation, which isn't really true; it just inherits an implementation.) This all applies to nsQuoteList, too. (This comment is copypasted there.) >+++ b/layout/base/nsFrameManager.cpp >+ size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { >+ size_t total = aMallocSizeOf(this); >+ // The table don't own the objects that it points to, so we don't need to >+ // pass a callback to measure those object. s/don't own/doesn't own/ s/object/objects/ >+/* static */ size_t >+nsFrameManagerBase::SizeOfIncludingThisOfUndisplayedMap(const UndisplayedMap* aMap, >+ mozilla::MallocSizeOf aMallocSizeOf) >+{ >+ return aMap->SizeOfIncludingThis(aMallocSizeOf); >+} >+ This seems a bit silly. Can't we just call aMap->SizeOfIncludingThis() directly in the place where we call this method? (If not, I'll bet it's just because the caller lives in a .h file. But it probably should live in this .cpp file, which would make this function unnecessary.) >diff --git a/layout/base/nsFrameManagerBase.h b/layout/base/nsFrameManagerBase.h > class nsFrameManagerBase > { >+protected: >+ class UndisplayedMap; >+ static size_t >+ SizeOfIncludingThisOfUndisplayedMap(const UndisplayedMap* aMap, >+ mozilla::MallocSizeOf aMallocSizeOf); >+ (Then this ^ whole added chunk could go away.) > public: > nsFrameManagerBase() > { >- memset(this, '\0', sizeof(nsFrameManagerBase)); >+ //memset(this, '\0', sizeof(nsFrameManagerBase)); > } Why are you commenting this out? >+ virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { >+ size_t total = 0; >+ // mPlaceholderMap and mUndisplayedMap don't own the objects that they >+ // point to, so we don't need to pass a callback to measure those objects. >+ total += PL_DHashTableSizeOfExcludingThis(&mPlaceholderMap, nullptr, aMallocSizeOf); >+ if (mUndisplayedMap) { >+ total += SizeOfIncludingThisOfUndisplayedMap(mUndisplayedMap, aMallocSizeOf); >+ } >+ return total; >+ } As noted above, I think impl should just go in the .cpp file, and then we won't need to bother with having a dedicated SizeOfIncludingThisOfUndisplayedMap helper. >diff --git a/layout/base/nsGenConList.h b/layout/base/nsGenConList.h >@@ -55,16 +56,26 @@ struct nsGenConNode : public PRCList { > { > mPseudoFrame = aPseudoFrame; > CheckFrameAssertions(); > return false; > } > > virtual ~nsGenConNode() {} // XXX Avoid, perhaps? > >+ virtual size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { >+ // We don't own mPseudoFrame so we don't count its size. >+ size_t total = 0; >+ if (mText) { >+ total += mText->SizeOfIncludingThis(aMallocSizeOf); Note that mText is a nsRefPtr (with your previous patch), so we might not be the sole owner. Are you sure it's not being reported as part of the content tree somehow? (and would potentially be double-reported) Note that nsCSSFrameConstructor captures the return-value of CreateGeneratedContent() (which I think is this text node) and passes it to AppendChildTo() on the container element, so it seems reasonable to suspect that it might already be reported there... Probably worth a brief comment-explanation in the code here, whether we should or shouldn't report it. (Also, if we shouldn't report it here, then the previous deCOM patch doesn't really belong as part of this bug, though maybe it doesn't matter.) >@@ -99,11 +110,28 @@ public: >+ // Not virtual because our two sub-classes (nsQuoteList and nsCounterList) >+ // don't have anything they would count in an "ExcludingThis" method, and we >+ // want to avoid bloating instances of this class with a vtable pointer. >+ size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { >+ size_t total = 0; >+ if (mFirstNode) { >+ for (nsGenConNode *node = Next(mFirstNode); node != mFirstNode; >+ node = Next(mFirstNode)) { >+ total += node->SizeOfIncludingThis(aMallocSizeOf); >+ } >+ } Nit: I don't think this considers the memory used by mFirstNode itself. At least, nsGenConList::Clear() iterates over the list in this same way, and it has a separate line (after the loop) to handle mFirstNode. So I'll bet we need: total += mFirstNode->SizeOfIncludingThis(aMallocSizeOf); >+ // SizeOfExcludingThis is not implemented. It would have to be virtual since >+ // our sub-classes implement it, and we want to avoid bloating instances of >+ // this class with a vtable pointer. You mean "SizeOfIncludingThis" here. >diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h >+ size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; >+ > private: >@@ -794,18 +796,16 @@ private: >- size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; >- This change may not be necessary, depending on the outcome of my comment about mCounterStyle above.

Comment on attachment 8438483 [details] [diff] [review] deCOMify nsGenConNode::mText so that we can measure its size https://hg.mozilla.org/integration/mozilla-inbound/rev/76d75daedc2a

Comment on attachment 8438766 [details] [diff] [review] explicitly initialize members in nsFrameManagerBase's ctor rather than using memset Why is this change needed?

To be able to add the methods to that class in this patch: https://bugzilla.mozilla.org/attachment.cgi?id=8438484&action=diff#a/layout/base/nsFrameManagerBase.h_sec3 Otherwise the memset would overwrite the vtable.

Comment on attachment 8438766 [details] [diff] [review] explicitly initialize members in nsFrameManagerBase's ctor rather than using memset Ah, there we go. Don't you need to set mPlaceholderMap.ops to null as well? I don't see how things can possibly work otherwise. r=me with that.

Yes, I definitely do. Thanks for the review!

Comment on attachment 8438484 [details] [diff] [review] PresShell::mFrameConstructor and all the stuff it owns (I suppose the nsCSSFrameConstructor patch is r=me with the various things in comment 32 addressed.) (One other note, though -- the new "part 4" patch (using init list instead of memset) might be better-labeled "part 2.5", since it really needs to land before part 3 in your patch-stack [the patch that gives us a vtable pointer, which the memset stomps on]. Anyway, as long as they land in that order, the numbering doesn't really matter.)

Comment on attachment 8438766 [details] [diff] [review] explicitly initialize members in nsFrameManagerBase's ctor rather than using memset https://hg.mozilla.org/integration/mozilla-inbound/rev/888eca7580fc

Comment on attachment 8429976 [details] [diff] [review] PresShell::mSelection Cancelling this one for the moment while I take another look at related DMD output.

Comment on attachment 8429977 [details] [diff] [review] PresShell::mCaret >diff --git a/content/base/public/FragmentOrElement.h b/content/base/public/FragmentOrElement.h >+ virtual size_t SizeOfOnlyThis(mozilla::MallocSizeOf aMallocSizeOf) const; MOZ_OVERRIDE >diff --git a/xpcom/glue/nsWeakReference.cpp b/xpcom/glue/nsWeakReference.cpp >+ virtual size_t SizeOfOnlyThis(mozilla::MallocSizeOf aMallocSizeOf) const; MOZ_OVERRIDE >diff --git a/xpcom/threads/nsTimerImpl.h b/xpcom/threads/nsTimerImpl.h >+ virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; MOZ_OVERRIDE I'm a little skeptical of the SizeOfOnlyThis methods, although I guess it's the right thing to do given that you can't assume that the nsIWeakReference sub-object is at the same location as the allocation (although in reality I think it is -- which would allow you to call aMallocSizeOf directly on the nsIWeakReference* -- and that's assuming that aMallocSizeOf doesn't deal with inner pointers). I'm also a little skeptical of adding code to measure such small things. It almost feels like the code will take up more memory than the stuff it's measuring, although I suppose that's not quite true in that there's one caret per pres shell. But I guess it's ok. r=dbaron, and sorry for the delay getting to this

Comment on attachment 8429977 [details] [diff] [review] PresShell::mCaret https://hg.mozilla.org/integration/mozilla-inbound/rev/5c26dd124145

The leave-open keyword is there and there is no activity for 6 months. :jwatt, maybe it's time to close this bug?

Yeah, let's call this done. I'll open a new bug if I ever get around to reviving the patches that didn't land.