At least for Links, I'd think. Bug 678376 comment 10 has URIs near the top of the list.

I mentioned using moz_malloc_usable_size in memory reporters elsewhere. Any interest in doing that now? It would save me having to retrofit it onto this reporter later :)

Do we want to have a single URI reporter? Or have the DOM report the URIs it points to? In particular, some of our bigger URIs (e.g. data:) would not be covered by the reporter added here... and there is no way to do per-page blame.

(In reply to Boris Zbarsky (:bz) from comment #3) > Do we want to have a single URI reporter? Or have the DOM report the URIs > it points to? The latter sounds like it would be a ton of work. > In particular, some of our bigger URIs (e.g. data:) would not be covered by > the reporter added here... and there is no way to do per-page blame. We could work nsSimpleURI into this scheme too. But yes, there is no way to do per-page blame with this approach.

> The latter sounds like it would be a ton of work. Would it, though? If we just add reporting for Link elements, that will cover the vast majority of them, I suspect... we can use DMD to find others as needed. The benefits of that approach include not adding two words to every URI object, not having the extra overhead of managing the list in non-debug builds, and per-page blame....

I guess one question would be how exactly to get someone holding an nsIURI to report its memory, though.

The other fun part will be avoiding double reporting.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > The other fun part will be avoiding double reporting. The good news is that DMD detects double reporting, once I teach it about the reporters.

If you just report the Link nsIURIs, you won't double-report.

(In reply to Boris Zbarsky (:bz) from comment #9) > If you just report the Link nsIURIs, you won't double-report. Ok. I'd set the dependency stuff but I can't find that bug.

> > If you just report the Link nsIURIs, you won't double-report. > > Ok. I'd set the dependency stuff but I can't find that bug. I can't find any bug about memory reporters for Link. If neither bz nor I can find such a bug, it might as well not exist. I can take this bug, DMD indicates that URIs are a sizeable chunk of what's left to report. Based on a conversation with bz, here's my rough plan: - |nsCOMPtr<nsIURI> Link::mCachedURI| is probably the most important case to catch for URIs. - Because |nsIURI| is just an interface, I'll need to add |SizeOf{In,Ex}cludingThis| to that interface. That requires doing the "HACK" in xpcom/io/nsBinaryStream.cpp when the IID changes. - nsHTMLAnchorElement, nsHTMLAreaElement, and nsHTMLLinkElement all inherit from Link. - The important sub-classes of nsIURI are: * nsSimpleURI, which is easy because it inherits directly from nsIURI. * nsStandardURL, which harder because it inherits from nsIRUI via nsIURL and nsIFileURL; those interfaces will need to have |SizeOf{In,Ex}cludingThis| added as well.

> * nsStandardURL, which harder because it inherits from nsIRUI via nsIURL and > nsIFileURL; those interfaces will need to have |SizeOf{In,Ex}cludingThis| added as well. I don't think that's necessary. Just putting it on nsIURI and implementing in nsStandardURL (well, and whatever other nsIURI impls we have) will work. nsIURL and nsIFileURL are abstract classes; there are no instances of them that get created, and they're full of pure virtual methods already; adding a few more won't hurt anything.

This small patch tweaks the size reporters for nsString and nsCString: - Adds 'const' to all the methods, which is needed sometimes. - Now measures strings that are F_SHARED but !IsReadonly(). This doesn't help with URIs, unfortunately, see below. I had to move the methods into the .cpp file to make IsReadonly() visible.

This patch adds measurement of URIs within the following classes that inherit from |Link|: nsHTMLAnchorElement, nsHTMLAreaElement, nsHTMLLinkElement. On biesi's advice I ended up not modifying nsIRUI, but instead implemented a new C++ interface, nsISizeOf, and doing some manual QIing where necessary. The classes that implement nsISizeOf are nsStandardURL, nsSimpleURI and nsNullPrincipalURI. There are some others (e.g. nsIconURI, nsJARURI) that could implement it but DMD says they're not worth the effort. With 8 news sites open, the new reporters measure about 800KB of URI stuff; that's from the nsIURI objects themselves... unfortunately, the strings within URIs are only measured if they're unshared, because I don't have a way to measure shared strings at the moment. And in practice they all are shared. Here's an example DMD report showing what's being missed (this is nsStandardURL::mSpec): Unreported: 4,788 block(s) in record 4 of 18724 534,976 bytes (480,821 requested / 54,155 slop) 0.44% of the heap (5.63% cumulative unreported) at malloc (vg_replace_malloc.c:263) by moz_malloc (mozalloc.cpp:113) by nsStringBuffer::Alloc(unsigned long) (nsSubstring.cpp:209) by nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) (nsTSubstring.cpp:162) by nsACString_internal::SetCapacity(unsigned int) (nsTSubstring.cpp:549) by nsACString_internal::SetLength(unsigned int) (nsTSubstring.cpp:579) by bool EnsureStringLength<nsCString>(nsCString&, unsigned int) (nsReadableUtils.h:392) by nsStandardURL::BuildNormalizedSpec(char const*) (nsStandardURL.cpp:585) by nsStandardURL::SetSpec(nsACString_internal const&) (nsStandardURL.cpp:1190) by nsStandardURL::Init(unsigned int, int, nsACString_internal const&, char const*, nsIURI*) (nsStandardURL .cpp:2683) I'm asking for lots of additional feedback: khuey because I cribbed a bunch of code off you; jlebar, bz, because this ties directly into our IRC conversation about |this| pointers. Feel free to ignore, but any feedback would be welcome.

Comment on attachment 596918 [details] [diff] [review] strings fixup bz found an obvious stupidity in the patch, due to some old code I forgot to remove. I'll put up a new version tomorrow, and hopefully the string measurement will be more successful as a result.

(In reply to Nicholas Nethercote from comment #13) > - Now measures strings that are F_SHARED but !IsReadonly(). This doesn't > help with URIs, unfortunately, see below. Would it help to divide the allocation of the buffer by the number of owners?

Why is this necessary? > #define NS_DEFINE_SIZEOF_EXCLUDING_THIS_FROM_SUPERCLASS(SuperT) \ > virtual NS_MUST_OVERRIDE size_t \ > SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const { \ > return SuperT::SizeOfExcludingThis(aMallocSizeOf); \ > } Can't we just inherit from the superclass? > + // SizeOfIncludingThis doesn't need to be overridden by sub-classes. I think it would be helpful to at least try to explain what's going on. Something like, "because all direct and indirect subclasses of nsINode inherit first from nsINode. Thus, casting from nsINode to a subclass does not adjust the |this| pointer which nsINode::SizeOfIncludingThis passes to malloc_usable_size()."

> On biesi's advice I ended up not modifying nsIRUI, but instead implemented a > new C++ interface, nsISizeOf, and doing some manual QIing where necessary. Are you sure you qref'ed? There's no nsISizeOf in this patch.

> > + // SizeOfIncludingThis doesn't need to be overridden by sub-classes. > > I think it would be helpful to at least try to explain what's going on. > Something like, "because all direct and indirect subclasses of nsINode > inherit first from nsINode. Thus, casting from nsINode to a subclass does > not adjust the |this| pointer which nsINode::SizeOfIncludingThis passes to > malloc_usable_size()." Yes, I'm planning to add that. When I wrote the patch I didn't understand all the subtleties of |this| pointers!

Attach the right patch this time.

Comment on attachment 597176 [details] [diff] [review] the main event (the right patch) Sorry for the bug churn, I'll have another version of this soon.

Turns out most of the strings within URIs aren't shared, which is good news.

New version. Something I'm very unsure about... - |Link| has a field |nsCOMPtr<nsIURI> mCachedURI|. - If |mCachedURI| points to an |nsSimpleURI| object, then |Link|'s SizeOfExcludingThis() will call nsISizeOf::GetSizeOf(), which will return an |nsISizeOf*| that points to the start of the |nsSimpleURI| object, and we can call nsISizeOf::SizeOfIncludingThis() safely on that pointer. - But what if |mCachedURI| points to an |nsBlobURI|, which is a sub-class of |nsSimpleURI| that doesn't implement |nsISizeOf|, and doesn't use |NS_SIZEOF_INTERFACE_MAP_ENTRY|. What will nsISizeOf::GetSize() return -- nsnull, or a pointer? If it's the latter, can nsISizeOf::SizeOfIncludingThis() be called safely?

Comment on attachment 597306 [details] [diff] [review] the main event, v2 Review of attachment 597306 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsSimpleURI.h @@ +82,5 @@ > + // - nsJSURI: mBaseURI > + // - nsSimpleNestedURI: mInnerURI > + // - nsBlobURI: mPrincipal > + // Note that none of these classes are currently measured at all because > + // they do not implement the nsISizeOf interface. They do implement the interface actually, because nsSimpleURI does and they use NS_INTERFACE_MAP_END_INHERITING or equivalent. So I think this should be OK, except of course that it doesn't count the members you mention in this comment.

+ /** + * Like SizeOfExcludingThis, but also includes the size of the object itself. + * Every direct sub-class must override this, even though the definition + * is always the same, otherwise the |this| pointer won't point to the start + * of the relevant object due to the QI in GetSizeOf(). + */ + virtual NS_MUST_OVERRIDE size_t + SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const { + return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf); + } Why not make this pure-virtual? The only reason to use NS_MUST_OVERRIDE is if you're going to explicitly call this function (i.e., nsISize::SizeOfIncludingThis()). But that call is never correct.

Comment on attachment 597306 [details] [diff] [review] the main event, v2 + static inline nsISizeOf* GetSizeOf(nsISupports* aSupports) + { + nsISizeOf* iface; + nsresult rv = CallQueryInterface(aSupports, &iface); + return NS_SUCCEEDED(rv) ? iface : nsnull; + } CallQueryInterface addrefs, so this should be a ticket to leak the world.

Oh, except you return early from QI with NS_SIZEOF_INTERFACE_MAP_ENTRY, I see. That's really confusing, and violates all our rules. Did you find that we can't afford these addref/releases, for some reason?

Comment on attachment 597304 [details] [diff] [review] strings fixup, v2 r=me

Comment on attachment 597306 [details] [diff] [review] the main event, v2 >+++ b/content/base/src/Link.cpp >+Link::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const >+ // Measurement of the following members may be added later if DMD finds it >+ // is worthwhile: >+ // - mLinkState >+ // - mElement >+ // - mHistory mLinkState is a data member. It'd be counted as part of sizeof(this). mElement is a pointer-to-self, basically, to avoid QIs. mHistory is a service; we don't own it. So we're never going to add measurement of those members. >+++ b/netwerk/base/src/nsStandardURL.cpp >+nsStandardURL::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const >+ // Measurement of the following members may be added later if DMD finds it is >+ // worthwhile: >+ // - mParser >+ // - mFile >+ // - mHostA There's no reason not to add mHostA now, right? It's just another string; no special code needed. I also don't see a reason to special-case the QI handling for nsISizeOf. Just make it inherit from nsISupports, make the SizeOfIncludingThis on it pure-virtual, and use the normal QI machinery to get it. In my opinion.

(In reply to Justin Lebar [:jlebar] from comment #27) > Oh, except you return early from QI with NS_SIZEOF_INTERFACE_MAP_ENTRY, I > see. > > That's really confusing, and violates all our rules. Did you find that we > can't afford these addref/releases, for some reason? (In reply to Boris Zbarsky (:bz) from comment #29) > > I also don't see a reason to special-case the QI handling for nsISizeOf. > Just make it inherit from nsISupports, make the SizeOfIncludingThis on it > pure-virtual, and use the normal QI machinery to get it. In my opinion. When khuey was helping me with this he suggested two possible approaches -- the one in the patch (which was inspired by some prior art... nsWrapperCache, maybe?), or to inherit from nsISupports. I chose the former because it seemed easier, but it looks like I made a bad choice :) This subtlety of |this|-pointer adjustment in relation to this patch is making me nervous about the whole endeavour, unfortunately. But I'll try the nsISupports approach, hopefully it'll be nicer.

nsWrapperCache is the way it is because: 1) Those QIs happened in performance-critical code, and the refcounting overhead would have been very noticeable. 2) At the time nsWrapperCache didn't have any virtual methods, and adding some (e.g. via inheriting it from nsISupports) would have bloated every DOM node by a word... Sadly, we added such a virtual method since then. :(

This version makes nsISizeOf inherit from nsISupports, and addresses the other comments. Please ignore the njn/MyaMallocSizeOf stuff, that's temporary stuff that makes it easy to identify the relevant reports in DMD's output, I'll change it back to aMallocSizeOf before I land anything.

>diff --git a/caps/src/nsNullPrincipalURI.h b/caps/src/nsNullPrincipalURI.h >--- a/caps/src/nsNullPrincipalURI.h >+++ b/caps/src/nsNullPrincipalURI.h >@@ -40,30 +40,36 @@ > /** > * This wraps nsSimpleURI so that all calls to it are done on the main thread. > */ > > #ifndef __nsNullPrincipalURI_h__ > #define __nsNullPrincipalURI_h__ > > #include "nsIURI.h" >+#include "nsISizeOf.h" > #include "nsAutoPtr.h" > #include "nsString.h" > > // {51fcd543-3b52-41f7-b91b-6b54102236e6} > #define NS_NULLPRINCIPALURI_IMPLEMENTATION_CID \ > {0x51fcd543, 0x3b52, 0x41f7, \ > {0xb9, 0x1b, 0x6b, 0x54, 0x10, 0x22, 0x36, 0xe6} } > > class nsNullPrincipalURI : public nsIURI >+ , public nsISizeOf One concern I have here is that we're adding a second vtable entry for every URI object. There are a lot of URI objects, right? Enough so that the size of the vtable matters? >+size_t >+Link::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const >+{ >+ size_t n = 0; >+ >+ if (mCachedURI) { >+ nsISizeOf* iface = nsISizeOf::GetSizeOf(mCachedURI); This needs to be QI now. >+ static inline nsISizeOf* GetSizeOf(nsISupports* aSupports) >+ { >+ nsISizeOf* iface; >+ nsresult rv = CallQueryInterface(aSupports, &iface); >+ return NS_SUCCEEDED(rv) ? iface : nsnull; >+ } Don't need this anymore. >+}; >+NS_DEFINE_STATIC_IID_ACCESSOR(nsISizeOf, NS_ISIZEOF_IID) >+ >+// This must be put in the NS_INTERFACE_MAP_BEGIN/NS_INTERFACE_MAP_END section >+// of any class that implements nsISizeOf. >+#define NS_SIZEOF_INTERFACE_MAP_ENTRY \ >+ if ( aIID.Equals(NS_GET_IID(nsISizeOf)) ) { \ >+ *aInstancePtr = static_cast<nsISizeOf*>(this); \ >+ return NS_OK; \ >+ } else Or this.

> There are a lot of URI objects, right? Enough so that the size of the vtable matters? It only matters if allocated object size increased... and at least for a nsStandardURL the one extra word is tiny compared to the URI itself.

(In reply to Justin Lebar [:jlebar] from comment #33) > > class nsNullPrincipalURI : public nsIURI > >+ , public nsISizeOf > > One concern I have here is that we're adding a second vtable entry for every > URI object. > > There are a lot of URI objects, right? Enough so that the size of the > vtable matters? It'd be nice to have smaller vtables, but the vtable is shared between objects. So this is just a couple of additional pointers total, not a couple of additional pointers per URI object. (It's per URI class, but that's not large in the grand scheme of things.)

Agreed, the vtables themselves are negligible. I'm only concerned with the vtable *pointers* in each object. Because this is multiple inheritance, don't we have one vtable pointer per inherited class, so two in total? If class A : B, C, then class A has to contain something which looks exactly like a B and something that looks exactly like a C . So we have to have one vtable pointer for B, and a separate vtable pointer for C.

Oh, indeed. bz's comment is of course more applicable, then.

> > >+size_t > >+Link::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const > >+{ > >+ size_t n = 0; > >+ > >+ if (mCachedURI) { > >+ nsISizeOf* iface = nsISizeOf::GetSizeOf(mCachedURI); > > This needs to be QI now. > > >+ static inline nsISizeOf* GetSizeOf(nsISupports* aSupports) > >+ { > >+ nsISizeOf* iface; > >+ nsresult rv = CallQueryInterface(aSupports, &iface); > >+ return NS_SUCCEEDED(rv) ? iface : nsnull; > >+ } > > Don't need this anymore. Do you just want me to inline GetSizeOf()?

Landed just the first patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/c714722a7aad Mergers, please keep this bug open, there's another patch to come.

I mean, it's just nsCOMPtr<nsISizeOf> foo = do_QueryInterface(bar) as opposed to nsCOMPtr<nsISizeOf> foo = nsISizeOf::GetSizeOf(bar). As written, GetSizeOf isn't correct because it addrefs but returns a raw pointer (as opposed to an already_addrefed<nsISizeOf>).

Comment on attachment 598765 [details] [diff] [review] the main event, v3 Comment 40 is spot on, and NS_SIZEOF_INTERFACE_MAP_ENTRY is unused so can go away. The rest looks good. I assume NS_DECL_SIZEOF_EXCLUDING_THIS is defined in some other patch somewhere?

> I assume NS_DECL_SIZEOF_EXCLUDING_THIS is defined in some other patch > somewhere? Yes, in bug 723799. Sorry if that wasn't clear.

> Don't need this anymore. > > >+NS_DEFINE_STATIC_IID_ACCESSOR(nsISizeOf, NS_ISIZEOF_IID) > >+ > >+// This must be put in the NS_INTERFACE_MAP_BEGIN/NS_INTERFACE_MAP_END section > >+// of any class that implements nsISizeOf. > >+#define NS_SIZEOF_INTERFACE_MAP_ENTRY \ > >+ if ( aIID.Equals(NS_GET_IID(nsISizeOf)) ) { \ > >+ *aInstancePtr = static_cast<nsISizeOf*>(this); \ > >+ return NS_OK; \ > >+ } else > > Or this. Do I need the NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISIZEOF_IID) within the class and the NS_DEFINE_STATIC_IID_ACCESSOR(nsISizeOf, NS_ISIZEOF_IID) afterwards?

I think you need that so one can QI to your interface, but if it compiles without...