write a memory reporter for the cookie service

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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...
Attachment #8343257 - Attachment is obsolete: true
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.
Attachment #8343261 - Flags: review?(n.nethercote)
Attachment #8343261 - Flags: review?(honzab.moz)
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 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 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...
Attachment #8343261 - Flags: review?(honzab.moz) → review?(ehsan)
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.
Attachment #8343261 - Flags: review?(ehsan) → review-
Addressed all the comments, I believe.
Attachment #8343261 - Attachment is obsolete: true
Attachment #8343261 - Flags: review?(n.nethercote)
Attachment #8343947 - Flags: review?(n.nethercote)
Attachment #8343947 - Flags: review?(ehsan)
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?
Attachment #8343947 - Flags: review?(n.nethercote) → review+
Attachment #8343947 - Flags: review?(ehsan) → review+
(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.
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/6a5f515fdda1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.