946870 - write a memory reporter for the cookie service

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Nathan Froyd [:froydnj]]

I swear that I had one DMD run where the cookie service was taking up a bunch of memory...but after spending some time writing a memory reporter for the cookie service, hardly anything is reported in about:memory.  Oh well, it's written now...

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: make the cookie service report its memory

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: make the cookie service report its memory

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8343261 [details] [diff] [review]
make the cookie service report its memory

Honza for the networking knowledge, Nick to make sure the memory reporting bits look sane.  Not really sure who should review this on the networking side.

The only tricky part is the measurement of DBState; I think it's OK to measure all the members, as the bits in readSet and hostArray shouldn't (AFAICS) overlap with the bits in hostTable.  DMD tells me I'm not double-reporting things doing it this way, at least, but I don't know that my testing profile has all that many cookies.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8343261 [details] [diff] [review]
make the cookie service report its memory

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

Thanks for writing this.

::: netwerk/cookie/nsCookieService.cpp
@@ +636,5 @@
> +  return amount;
> +}
> +
> +static size_t
> +ReadSetEntrySizeOfExcludingThis(nsCookieKey *aEntry,

This looks exactly the same as HostTableEntrySizeOfExcludingThis. Maybe just consolidate to EntrySizeOfExcludingThis?

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8343261 [details] [diff] [review]
make the cookie service report its memory

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

FWIW, I too have sometimes seen moderately large numbers in DMD output for the cookie service.

::: netwerk/cookie/nsCookieService.cpp
@@ +606,5 @@
> +
> +size_t
> +nsCookieEntry::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf)
> +{
> +  size_t amount = nsCookieKey::SizeOfExcludingThis(aMallocSizeOf);

s/amount/n/ ?

@@ +650,5 @@
> +  size_t amount = 0;
> +
> +  amount += aMallocSizeOf(this);
> +  amount += hostTable.SizeOfExcludingThis(HostTableEntrySizeOfExcludingThis,
> +                                          aMallocSizeOf, nullptr);

Can you omit the nullptr arg, here and below?  I can't remember if there's a default and it's Friday night and I'm too lazy to look it up...

@@ +722,5 @@
>                     nsICookieManager,
>                     nsICookieManager2,
>                     nsIObserver,
> +                   nsISupportsWeakReference,
> +                   nsIMemoryReporter)

This needs to be:

NS_IMPL_ISUPPORTS_INHERITED5(nsCookieService,
                             MemoryUniReporter,
                             nsICookieService,
                             nsICookieManager,
                             nsICookieManager2,
                             nsIObserver,
                             nsISupportsWeakReference)

The INHERITED is because MemoryUniReporter is a non-abstract class that implements nsISupport.  (At least, that's how all the other reporters that inherit from MemoryUniReporter do it.)

@@ +766,5 @@
>  
>    // Init our default, and possibly private DBStates.
>    InitDBStates();
>  
> +  RegisterWeakMemoryReporter(this);

You need an UnregisterWeakMemoryReporter(this) call too, probably in nsCookieService's destructor.  Otherwise, when the nsCookieService is destroyed, the memory reporter manager will end up holding a weak ref to an object that no longer exists.

@@ +4338,5 @@
>    }
>  }
>  
> +int64_t
> +nsCookieService::Amount()

It's a bit pedantic, but I would make the contents of this function into the SizeOfIncludingThis() function (which returns size_t), and then make the Amount() function (which returns int64_t) just call SizeOfIncludingThis().

::: netwerk/cookie/nsCookieService.h
@@ +234,5 @@
>  class nsCookieService : public nsICookieService
>                        , public nsICookieManager2
>                        , public nsIObserver
>                        , public nsSupportsWeakReference
> +                      , public mozilla::MemoryUniReporter

Put the MemoryUniReporter first in the list, to match the INHERITED macro above.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8343261 [details] [diff] [review]
make the cookie service report its memory

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

Forwarding to ehsan, as he is more involved in this code.  I don't know the cookie service at all!

Ehsan, if you don't want to do this, bounce back.

::: netwerk/cookie/nsCookie.cpp
@@ +114,5 @@
> +size_t
> +nsCookie::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +{
> +    // There is no need to measure the sizes of the individual string
> +    // members, since the strings are stored in-line with the nsCookie.

and how are the string buffers reported? (I'm not that familiar with memory reporter)

::: netwerk/cookie/nsCookieService.h
@@ +103,5 @@
>      temp.Append(aKey->mInBrowserElement ? 1 : 0);
>      return mozilla::HashString(temp);
>    }
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf);

Just make sure this doesn't need to be virtual.

@@ +243,3 @@
>    public:
>      // nsISupports
>      NS_DECL_ISUPPORTS

It's so bad that MemoryUniReporter implements nsISupports, and does so as thread-safe...

Comment 7

[(no longer active)]

Comment on attachment 8343261 [details] [diff] [review]
make the cookie service report its memory

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

::: netwerk/cookie/nsCookie.cpp
@@ +114,5 @@
> +size_t
> +nsCookie::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +{
> +    // There is no need to measure the sizes of the individual string
> +    // members, since the strings are stored in-line with the nsCookie.

Note to Honza:  This is fine, see nsCookie::Create.

@@ +115,5 @@
> +nsCookie::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +{
> +    // There is no need to measure the sizes of the individual string
> +    // members, since the strings are stored in-line with the nsCookie.
> +    return aMallocSizeOf(this);

Please add a comment to nsCookie.h where the members are declared saying that this function should be updated if the storage strategy for nsCookie changes in the future.

::: netwerk/cookie/nsCookieService.cpp
@@ +722,5 @@
>                     nsICookieManager,
>                     nsICookieManager2,
>                     nsIObserver,
> +                   nsISupportsWeakReference,
> +                   nsIMemoryReporter)

Wait, that's crazy.  That means that we're going to have two refcounts per any object that derives from MemoryUniReporter.  r- pending at least an explanation on why that's OK.

@@ +4344,5 @@
> +  size_t amount = 0;
> +
> +  amount += MallocSizeOf(this);
> +  amount += mDefaultDBState->SizeOfIncludingThis(MallocSizeOf);
> +  amount += mPrivateDBState->SizeOfIncludingThis(MallocSizeOf);

Please null check both mDefaultDBState and mPrivateDBState to make sure that this won't crash if called after CloseDBStates().

::: netwerk/cookie/nsCookieService.h
@@ +103,5 @@
>      temp.Append(aKey->mInBrowserElement ? 1 : 0);
>      return mozilla::HashString(temp);
>    }
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf);

I don't think that it should.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: make the cookie service report its memory

Addressed all the comments, I believe.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8343947 [details] [diff] [review]
make the cookie service report its memory

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

::: netwerk/cookie/nsCookieService.cpp
@@ +765,5 @@
>  
>    // Init our default, and possibly private DBStates.
>    InitDBStates();
>  
> +  RegisterWeakMemoryReporter(this);

You still need to unregister |this| in the destructor.

@@ +4350,5 @@
> +  }
> +
> +  return n;
> +}
> +  

Trailing whitespace?

::: netwerk/cookie/nsCookieService.h
@@ +149,5 @@
>  {
>    nsCookieKey key;
>    nsRefPtr<nsCookie> cookie;
> +
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf);

Can the method be |const|?

@@ +162,5 @@
>    }
>  
>    NS_INLINE_DECL_REFCOUNTING(DBState)
>  
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);

Ditto?

@@ +238,5 @@
>                        , public nsSupportsWeakReference
>  {
> +  private:
> +    int64_t Amount() MOZ_OVERRIDE;
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);

Ditto?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #4)
> Thanks for writing this.

You're welcome!

> > +static size_t
> > +ReadSetEntrySizeOfExcludingThis(nsCookieKey *aEntry,
> 
> This looks exactly the same as HostTableEntrySizeOfExcludingThis. Maybe just
> consolidate to EntrySizeOfExcludingThis?

ReadSetEntrySizeOfExcludingThis has an nsCookieKey* argument; HostTableEntrySizeOfExcludingThis has an nsCookieEntry* argument.  So consolidating doesn't work.

Comment 11

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5f515fdda1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a5f515fdda1