682431 - Add memory reporters for URIs and Links

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Hook up the basics

Comment 2

[Nicholas Nethercote [inactive]]

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 :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The other fun part will be avoiding double reporting.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 11

[Nicholas Nethercote [inactive]]

> > 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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

>  * 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.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: strings fixup

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.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: the main event

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 15

[Nicholas Nethercote [inactive]]

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.

Comment 16

[[e-mail]]

(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?

Comment 17

[Justin Lebar (not reading bugmail)]

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()."

Comment 18

[Justin Lebar (not reading bugmail)]

> 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.

Comment 19

[Nicholas Nethercote [inactive]]

> > +  // 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!

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: the main event (the right patch)

Attach the right patch this time.

Comment 21

[Nicholas Nethercote [inactive]]

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.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: strings fixup, v2

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

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: the main event, v2

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 24

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 25

[Justin Lebar (not reading bugmail)]

+  /**
+   * 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 26

[Justin Lebar (not reading bugmail)]

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.

Comment 27

[Justin Lebar (not reading bugmail)]

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 28

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Nicholas Nethercote [inactive]]

(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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: the main event, v3

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.

Comment 33

[Justin Lebar (not reading bugmail)]

>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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 35

[Nathan Froyd [:froydnj]]

(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.)

Comment 36

[Justin Lebar (not reading bugmail)]

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.

Comment 37

[Nathan Froyd [:froydnj]]

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

Comment 38

[Nicholas Nethercote [inactive]]

> 
> >+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()?

Comment 39

[Nicholas Nethercote [inactive]]

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.

Comment 40

[Justin Lebar (not reading bugmail)]

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 42

[Ed Morley [:emorley]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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?

Comment 44

[Nicholas Nethercote [inactive]]

> I assume NS_DECL_SIZEOF_EXCLUDING_THIS is defined in some other patch
> somewhere?

Yes, in bug 723799.  Sorry if that wasn't clear.

Comment 45

[Nicholas Nethercote [inactive]]

> 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?

Comment 46

[Justin Lebar (not reading bugmail)]

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

Comment 47

[Nicholas Nethercote [inactive]]

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

Comment 48

[Matt Brubeck (:mbrubeck)]

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