Closed Bug 964039 Opened 6 years ago Closed 6 years ago

Memory used by the new cache backend is not reported

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: ehoogeveen, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 files, 3 obsolete files)

Running DMD on my usual profile, after browsing around a bit and letting it sit overnight, the new cache backend is the main source of unreported memory; attached is the relevant part of the DMD output (everything with cache2 directly responsible for the allocations, aside from instances of operator new that were part of a long tail). The vast majority comes from mozilla::net::CacheFileChunk::EnsureBufSize - that one stack is responsible for 20.57% of the unreported memory in the heap.
Whiteboard: [MemShrink]
No longer blocks: 931806
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8374443 - Flags: review?(michal.novotny)
Attachment #8374443 - Flags: review?(emanuel.hoogeveen)
Comment on attachment 8374443 [details] [diff] [review]
v1

Looks okay to me, but I'm not a peer (and njn should review memory reporter stuff anyway).
Attachment #8374443 - Flags: review?(emanuel.hoogeveen) → review?(n.nethercote)
Comment on attachment 8374443 [details] [diff] [review]
v1

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

Thanks for doing this. What kind of numbers are you getting? Some example output would be helpful.

The patch mostly looks good. But there's one big problem, so I'd like to see it again: there are several places where you compute memory sizes analytically, rather than measuring them with a MallocSizeOf function. This has two problems. First, if jemalloc rounded up the allocation request, you won't account for the extra bytes. Second, DMD won't know anything about the computation and will think that the relevant heap blocks are unreported. Please change these cases to use MallocSizeOf. I've marked all the ones I spotted below.

Also, have you done a DMD run? If not, it would be a good idea to do one, to verify that you're not double reporting anything, missing anything, etc. See https://wiki.mozilla.org/Performance/MemShrink/DMD for instructions. You can look in the "Unreported" section to make sure you're not missing anything, and in the "Once-reported" section to see the blocks that you are reporting are showing up as expected.

https://wiki.mozilla.org/Memory_Reporting would also be interesting reading if you haven't already seen it.

::: netwerk/cache2/CacheFile.cpp
@@ +1493,5 @@
> +    mChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf) +
> +    mCachedChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf) +
> +    (mMetadata ? mMetadata->SizeOfIncludingThis(mallocSizeOf) : 0) +
> +    (mHandle ? mHandle->SizeOfIncludingThis(mallocSizeOf) : 0) +
> +    mInputs.SizeOfExcludingThis(mallocSizeOf) +

For cases like this where there are lots of components, we typically write:

 size_t n = 0;
 n += ...;
 n += ...;
 ...
 return n;

But this is ok too, if you prefer it.

@@ +1496,5 @@
> +    (mHandle ? mHandle->SizeOfIncludingThis(mallocSizeOf) : 0) +
> +    mInputs.SizeOfExcludingThis(mallocSizeOf) +
> +    // The streams are only referencing other stuff that is reported elsewhere,
> +    // just report the number * size of instance for each of them.
> +    (mInputs.Length() ? mInputs.Length() * mallocSizeOf(mInputs[0]) : 0) +

This is a partly analytical computation. Please instead use mallocSizeOf to measure every block.

Also, I don't understand the ownership here... are the pointers in this array pointing to objects that are reported elsewhere? If so, they shouldn't be measured here. It's important not to double-count any blocks.

@@ +1497,5 @@
> +    mInputs.SizeOfExcludingThis(mallocSizeOf) +
> +    // The streams are only referencing other stuff that is reported elsewhere,
> +    // just report the number * size of instance for each of them.
> +    (mInputs.Length() ? mInputs.Length() * mallocSizeOf(mInputs[0]) : 0) +
> +    mallocSizeOf(mOutput);

This doesn't look right. |mOutput| is a CacheFileOutputStream. I think this should be |mOutput->SizeOfIncludingThis(mallocSizeOf)|.

::: netwerk/cache2/CacheFileChunk.cpp
@@ +676,5 @@
> +size_t
> +CacheFileChunk::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return
> +    MemorySize() + // Correctly reports size of all allocated buffers

Another analytical size computation. Please measure with mallocSizeof instead; create a new function for this purpose if necessary.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +119,5 @@
> +size_t
> +CacheFileHandle::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return
> +    SHA1Sum::HashSize +

Another analytical computation. I think |mallocSizeOf(mHash)| is better?

@@ +120,5 @@
> +CacheFileHandle::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return
> +    SHA1Sum::HashSize +
> +    mallocSizeOf(mFD);

This is a hack... ideally PRFileDesc would have its own SizeOfIncludingThis() function, but since it's a small struct, and modifying NSPR is a pain, this will do.

Any reason why you're not measuring |mKey| here? FWIW, it's common to write comments explaining why some fields aren't measured. Here's an example: http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRules.cpp?from=nsCSSRules.cpp#488

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +817,5 @@
> +
> +size_t
> +CacheFileMetadata::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return MemoryUsage();

Another analytical computation.

::: netwerk/cache2/CacheStorageService.cpp
@@ +609,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsCOMPtr<nsIObserverService> obsSvc = mozilla::services::GetObserverService();
>    if (obsSvc) {
> +    obsSvc->NotifyObservers(CacheStorageService::SelfISupports(),

I don't understand this change.

@@ +1502,5 @@
> +  CacheStorageService::Self()->Lock().AssertCurrentThreadOwns();
> +
> +  return
> +    mFrecencyArray.SizeOfExcludingThis(mallocSizeOf) +
> +    mExpirationArray.SizeOfExcludingThis(mallocSizeOf) +

Are the CacheEntry objects pointed to by these arrays reported elsewhere? I think they are; if so, it's worth a comment.

@@ +1525,5 @@
> +
> +size_t CollectEntryMemory(nsACString const & aKey,
> +                          nsRefPtr<mozilla::net::CacheEntry> const & aEntry,
> +                          mozilla::MallocSizeOf mallocSizeOf,
> +                          void * aClosure)

You're being inconsistent with how you use '*' in pointer types. This patch contains at least three variations:
- void* x
- void *x
- void * x

Please use one style consistently. Standard Mozilla style is |void* x|, so please use that unless there are compelling reasons not to.

@@ +1532,5 @@
> +
> +  CacheEntryTable* aTable = static_cast<CacheEntryTable*>(aClosure);
> +
> +  // Bypass memory-only entries, those will be reported when iterating
> +  // the memory only table. Memory only entries are stored in both ALL_ENTRIES

Add '-' between "Memory" and "only".

@@ +1533,5 @@
> +  CacheEntryTable* aTable = static_cast<CacheEntryTable*>(aClosure);
> +
> +  // Bypass memory-only entries, those will be reported when iterating
> +  // the memory only table. Memory only entries are stored in both ALL_ENTRIES
> +  // and MEMORY_ONLY hashtables.

I don't understand this ALL_ENTRIES and MEMORY_ONLY stuff. Hopefully it's clearer to somebody who is familiar with these data structures.

@@ +1555,5 @@
> +  data.mHandleReport->Callback(
> +    EmptyCString(),
> +    nsPrintfCString("explicit/network/cache2/%s-storage(%s)",
> +      aTable->Type() == CacheEntryTable::MEMORY_ONLY ? "memory" : "disk",
> +      aKey.BeginReading()),

Nice! Having a separate report for each kind of storage is good.

@@ +1559,5 @@
> +      aKey.BeginReading()),
> +    nsIMemoryReporter::KIND_HEAP,
> +    nsIMemoryReporter::UNITS_BYTES,
> +    size,
> +    NS_LITERAL_CSTRING("Memory used by cache storage"),

Add "the" before "cache", and add a '.' at the end of the sentence, please.

@@ +1578,5 @@
> +  // Report the service instance, this doesn't report entries, done lower
> +  rv = MOZ_COLLECT_REPORT(
> +    "explicit/network/cache2/service", KIND_HEAP, UNITS_BYTES,
> +    SizeOfIncludingThis(MallocSizeOf),
> +    "Memory used by cache storage service.");

Add "the" before "cache".

@@ +1583,5 @@
> +  if (NS_WARN_IF(rv))
> +    return rv;
> +
> +  // Report all entries, each storage separately (by the context key):
> +  // *CacheEntry->CacheFile->*CacheFileChunk+CacheFileMetadata+CacheFileHandle

I don't understand this comment, especially what the '*' and '->*' mean.

@@ +1587,5 @@
> +  // *CacheEntry->CacheFile->*CacheFileChunk+CacheFileMetadata+CacheFileHandle
> +  ReportStorageMemoryData data;
> +  data.mHandleReport = aHandleReport;
> +  data.mData = aData;
> +  sGlobalEntryTables->EnumerateRead(&ReportStorageMemory, &data);

You don't need the '&' in front of ReportStorageMemory.
Attachment #8374443 - Flags: review?(n.nethercote) → feedback+
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Comment on attachment 8374443 [details] [diff] [review]
> v1
> 
> Review of attachment 8374443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this. What kind of numbers are you getting? Some example
> output would be helpful.

Thanks for a quick r.

> 
> The patch mostly looks good. But there's one big problem, so I'd like to see
> it again: there are several places where you compute memory sizes
> analytically, rather than measuring them with a MallocSizeOf function. This
> has two problems. First, if jemalloc rounded up the allocation request, you
> won't account for the extra bytes. Second, DMD won't know anything about the
> computation and will think that the relevant heap blocks are unreported.
> Please change these cases to use MallocSizeOf. I've marked all the ones I
> spotted below.
> 
> Also, have you done a DMD run? 

No, I presume not supported on Win32?

> If not, it would be a good idea to do one, to
> verify that you're not double reporting anything, missing anything, etc. See
> https://wiki.mozilla.org/Performance/MemShrink/DMD for instructions. You can
> look in the "Unreported" section to make sure you're not missing anything,
> and in the "Once-reported" section to see the blocks that you are reporting
> are showing up as expected.
> 
> https://wiki.mozilla.org/Memory_Reporting would also be interesting reading
> if you haven't already seen it.

I think that is the one I was working with during the patch writing.

> 
> ::: netwerk/cache2/CacheFile.cpp
> @@ +1493,5 @@
> > +    mChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf) +
> > +    mCachedChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf) +
> > +    (mMetadata ? mMetadata->SizeOfIncludingThis(mallocSizeOf) : 0) +
> > +    (mHandle ? mHandle->SizeOfIncludingThis(mallocSizeOf) : 0) +
> > +    mInputs.SizeOfExcludingThis(mallocSizeOf) +
> 
> For cases like this where there are lots of components, we typically write:
> 
>  size_t n = 0;
>  n += ...;
>  n += ...;
>  ...
>  return n;

I like yours and wanted to do it that way.  But I was ones told not to do it :)  Again, missing general rule for styling...  Everybody says something different...

> 
> But this is ok too, if you prefer it.
> 
> @@ +1496,5 @@
> > +    (mHandle ? mHandle->SizeOfIncludingThis(mallocSizeOf) : 0) +
> > +    mInputs.SizeOfExcludingThis(mallocSizeOf) +
> > +    // The streams are only referencing other stuff that is reported elsewhere,
> > +    // just report the number * size of instance for each of them.
> > +    (mInputs.Length() ? mInputs.Length() * mallocSizeOf(mInputs[0]) : 0) +
> 
> This is a partly analytical computation. Please instead use mallocSizeOf to
> measure every block.

Sounds like you don't like mallocSizeOf being used outside an object and prefer SizeOf*This methods on all of them.  I'll do that.

> 
> Also, I don't understand the ownership here... are the pointers in this
> array pointing to objects that are reported elsewhere? If so, they shouldn't
> be measured here. It's important not to double-count any blocks.

Those are not elsewhere reported (AFAIK).  I'll check on it or it will be shown during DMD run.

> 
> @@ +1497,5 @@
> > +    mInputs.SizeOfExcludingThis(mallocSizeOf) +
> > +    // The streams are only referencing other stuff that is reported elsewhere,
> > +    // just report the number * size of instance for each of them.
> > +    (mInputs.Length() ? mInputs.Length() * mallocSizeOf(mInputs[0]) : 0) +
> > +    mallocSizeOf(mOutput);
> 
> This doesn't look right. |mOutput| is a CacheFileOutputStream. I think this
> should be |mOutput->SizeOfIncludingThis(mallocSizeOf)|.

As above.

> 
> ::: netwerk/cache2/CacheFileChunk.cpp
> @@ +676,5 @@
> > +size_t
> > +CacheFileChunk::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  return
> > +    MemorySize() + // Correctly reports size of all allocated buffers
> 
> Another analytical size computation. Please measure with mallocSizeof
> instead; create a new function for this purpose if necessary.

Yup.

> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +119,5 @@
> > +size_t
> > +CacheFileHandle::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  return
> > +    SHA1Sum::HashSize +
> 
> Another analytical computation. I think |mallocSizeOf(mHash)| is better?

Probably yes and also safer.  I've discovered how mallocSizeOf actually works later and didn't change this according that.

> 
> @@ +120,5 @@
> > +CacheFileHandle::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  return
> > +    SHA1Sum::HashSize +
> > +    mallocSizeOf(mFD);
> 
> This is a hack... ideally PRFileDesc would have its own
> SizeOfIncludingThis() function, but since it's a small struct, and modifying
> NSPR is a pain, this will do.

OK

> 
> Any reason why you're not measuring |mKey| here? FWIW, it's common to write
> comments explaining why some fields aren't measured. Here's an example:
> http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRules.
> cpp?from=nsCSSRules.cpp#488
 
If you tell me how I will gladly do!  But there is no SizeOf* on nsCString!  The example doesn't show much..

> 
> ::: netwerk/cache2/CacheFileMetadata.cpp
> @@ +817,5 @@
> > +
> > +size_t
> > +CacheFileMetadata::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  return MemoryUsage();
> 
> Another analytical computation.
> 
> ::: netwerk/cache2/CacheStorageService.cpp
> @@ +609,5 @@
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> >    nsCOMPtr<nsIObserverService> obsSvc = mozilla::services::GetObserverService();
> >    if (obsSvc) {
> > +    obsSvc->NotifyObservers(CacheStorageService::SelfISupports(),
> 
> I don't understand this change.

Michal should do this.  Anti-ambiguity static_cast is hidden in that method, we now derive from two interfaces...

> 
> @@ +1502,5 @@
> > +  CacheStorageService::Self()->Lock().AssertCurrentThreadOwns();
> > +
> > +  return
> > +    mFrecencyArray.SizeOfExcludingThis(mallocSizeOf) +
> > +    mExpirationArray.SizeOfExcludingThis(mallocSizeOf) +
> 
> Are the CacheEntry objects pointed to by these arrays reported elsewhere? I
> think they are; if so, it's worth a comment.

Are, and the comment is around here I think.  Actually just bellow the line ;)  I should move it to a better place.

> 
> @@ +1525,5 @@
> > +
> > +size_t CollectEntryMemory(nsACString const & aKey,
> > +                          nsRefPtr<mozilla::net::CacheEntry> const & aEntry,
> > +                          mozilla::MallocSizeOf mallocSizeOf,
> > +                          void * aClosure)
> 
> You're being inconsistent with how you use '*' in pointer types. This patch
> contains at least three variations:
> - void* x
> - void *x
> - void * x

There is plan for a bug to standardize this (a big overall review of all of this code) where we will do much more since styling rules has changed between this code started to be written and when we have merged it to m-c.

> 
> Please use one style consistently. Standard Mozilla style is |void* x|, so
> please use that unless there are compelling reasons not to.

There is one personal reason: I hate mozilla's styling guides and prefer space everywhere - I never know where the hell the asterisk or ampersand has to be.  But as mentioned above. . .

> 
> @@ +1532,5 @@
> > +
> > +  CacheEntryTable* aTable = static_cast<CacheEntryTable*>(aClosure);
> > +
> > +  // Bypass memory-only entries, those will be reported when iterating
> > +  // the memory only table. Memory only entries are stored in both ALL_ENTRIES
> 
> Add '-' between "Memory" and "only".
> 
> @@ +1533,5 @@
> > +  CacheEntryTable* aTable = static_cast<CacheEntryTable*>(aClosure);
> > +
> > +  // Bypass memory-only entries, those will be reported when iterating
> > +  // the memory only table. Memory only entries are stored in both ALL_ENTRIES
> > +  // and MEMORY_ONLY hashtables.
> 
> I don't understand this ALL_ENTRIES and MEMORY_ONLY stuff. Hopefully it's
> clearer to somebody who is familiar with these data structures.

Michal should do this.

> 
> @@ +1555,5 @@
> > +  data.mHandleReport->Callback(
> > +    EmptyCString(),
> > +    nsPrintfCString("explicit/network/cache2/%s-storage(%s)",
> > +      aTable->Type() == CacheEntryTable::MEMORY_ONLY ? "memory" : "disk",
> > +      aKey.BeginReading()),
> 
> Nice! Having a separate report for each kind of storage is good.
> 
> @@ +1559,5 @@
> > +      aKey.BeginReading()),
> > +    nsIMemoryReporter::KIND_HEAP,
> > +    nsIMemoryReporter::UNITS_BYTES,
> > +    size,
> > +    NS_LITERAL_CSTRING("Memory used by cache storage"),
> 
> Add "the" before "cache", and add a '.' at the end of the sentence, please.
> 
> @@ +1578,5 @@
> > +  // Report the service instance, this doesn't report entries, done lower
> > +  rv = MOZ_COLLECT_REPORT(
> > +    "explicit/network/cache2/service", KIND_HEAP, UNITS_BYTES,
> > +    SizeOfIncludingThis(MallocSizeOf),
> > +    "Memory used by cache storage service.");
> 
> Add "the" before "cache".
> 
> @@ +1583,5 @@
> > +  if (NS_WARN_IF(rv))
> > +    return rv;
> > +
> > +  // Report all entries, each storage separately (by the context key):
> > +  // *CacheEntry->CacheFile->*CacheFileChunk+CacheFileMetadata+CacheFileHandle
> 
> I don't understand this comment, especially what the '*' and '->*' mean.

It means relations: 1-n, 1-1 etc.

> 
> @@ +1587,5 @@
> > +  // *CacheEntry->CacheFile->*CacheFileChunk+CacheFileMetadata+CacheFileHandle
> > +  ReportStorageMemoryData data;
> > +  data.mHandleReport = aHandleReport;
> > +  data.mData = aData;
> > +  sGlobalEntryTables->EnumerateRead(&ReportStorageMemory, &data);
> 
> You don't need the '&' in front of ReportStorageMemory.

But I add it always.  I've met situations in the past this wouldn't compile.
Can you show some example output?

> > Also, have you done a DMD run? 
> 
> No, I presume not supported on Win32?

It does! Support was added recently.

> I like yours and wanted to do it that way.  But I was ones told not to do it
> :)  Again, missing general rule for styling...  Everybody says something
> different...

It's not a big deal either way.

> Sounds like you don't like mallocSizeOf being used outside an object and
> prefer SizeOf*This methods on all of them.

That's correct for most cases. Occasionally we'll have a trivial struct that only contains scalars and has no methods, in which case the SizeOf*This function(s) are overkill, but I think this case wasn't that simple.

> > Any reason why you're not measuring |mKey| here? FWIW, it's common to write
> > comments explaining why some fields aren't measured. Here's an example:
> > http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRules.
> > cpp?from=nsCSSRules.cpp#488
>  
> If you tell me how I will gladly do!  But there is no SizeOf* on nsCString! 
> The example doesn't show much..

The string hierarchy is really hard to understand, so it's not obvious that there are functions on types elsewhere in that can be used on nsCString:

- SizeOfExcludingThisMustBeUnshared() -- use this if you're certain that this is the only reference to the string. (There's an assertion to this effect.)

- SizeOfExcludingThisIfUnshared() -- use this is the string might be shared.

> There is one personal reason: I hate mozilla's styling guides and prefer
> space everywhere - I never know where the hell the asterisk or ampersand has
> to be.  But as mentioned above. . .

Whether you hate it or not, there is a style for pointers, which is |void* x|. We're moving towards a future where the style rules are applied more consistently (see recent dev-platform discussions), so if you're writing new code you might as well use |void* x| so it doesn't have to change again in the future.

> > > +  // Report all entries, each storage separately (by the context key):
> > > +  // *CacheEntry->CacheFile->*CacheFileChunk+CacheFileMetadata+CacheFileHandle
> > 
> > I don't understand this comment, especially what the '*' and '->*' mean.
> 
> It means relations: 1-n, 1-1 etc.

Really? I still don't understand.

I heard some wonderful advice a few years back: when somebody is reading something that you wrote, and they say "I don't understand this", don't take it as a cue to explain it in more detail. Instead, take it as a cue to rewrite it so it's clearer :)

Finally, I just want to say that despite all my comments, this was an excellent first version of a memory reporter patch, especially given that it was far from simple. There are lots of non-obvious little details with this stuff.
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Can you show some example output?
> 
> > > Also, have you done a DMD run? 
> > 
> > No, I presume not supported on Win32?
> 
> It does! Support was added recently.

My gum build (a <week old) says otherwise in about:memory, but I will update and see.  Thanks for the tip!

> The string hierarchy is really hard to understand, so it's not obvious that
> there are functions on types elsewhere in that can be used on nsCString:
> 
> - SizeOfExcludingThisMustBeUnshared() -- use this if you're certain that
> this is the only reference to the string. (There's an assertion to this
> effect.)
> 
> - SizeOfExcludingThisIfUnshared() -- use this is the string might be shared.

Aha!  Was not aware of them.  Now to decide which to use..

> > It means relations: 1-n, 1-1 etc.
> 
> Really? I still don't understand.
> 
> I heard some wonderful advice a few years back: when somebody is reading
> something that you wrote, and they say "I don't understand this", don't take
> it as a cue to explain it in more detail. Instead, take it as a cue to
> rewrite it so it's clearer :)

I know, I have a similar rule when on the other side :)  Will improve the comment.

> 
> Finally, I just want to say that despite all my comments, this was an
> excellent first version of a memory reporter patch, especially given that it
> was far from simple. There are lots of non-obvious little details with this
> stuff.

Thanks!
(In reply to Honza Bambas (:mayhemer) from comment #6)
> > > No, I presume not supported on Win32?
> > 
> > It does! Support was added recently.
> 
> My gum build (a <week old) says otherwise in about:memory, but I will update
> and see.  Thanks for the tip!
You have to do a build with DMD enabled (ac_add_options --enable-dmd), then run with some environment variables set. On Windows you also need ac_add_options --enable-profiling. See https://wiki.mozilla.org/Performance/MemShrink/DMD for more information. Incidentally I initially found this by running DMD on Windows!
Comment on attachment 8374443 [details] [diff] [review]
v1

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +119,5 @@
> +size_t
> +CacheFileHandle::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return
> +    SHA1Sum::HashSize +

Is this correct? Hash is stored in HandleHashKey and CacheFileHandle just holds a pointer to it. We can have more CacheFileHandles for a single HandleHashKey so we would count this memory several times.

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +817,5 @@
> +
> +size_t
> +CacheFileMetadata::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return MemoryUsage();

MemoryUsage() doesn't count mWriteBuf. It does exist only for a very short time during metadata writing. It is correct to ignore it for our internal memory reporting, but it should be reported via about:memory.
Attachment #8374443 - Flags: review?(michal.novotny) → review-
(In reply to Michal Novotny (:michal) from comment #8)
> Comment on attachment 8374443 [details] [diff] [review]
> v1

This is what I needed, good points, thanks!
So, I'm successfully running DMD on windows.  I'm checking the Unreported section for cache2 on the stack, and getting few stuff unreported that I actually don't know how to report:

- mozilla::Mutex
- any xpcom object that is exclusively referenced only by (leaf) objects in the sub-tree I report, e.g. nsIFile.

There is no way known to me to report them.  I understand that for the xpcom objects this is hard.  But at least the Mutex should be reported IMO.

Is there a way, or should I just leave it unreported?
Flags: needinfo?(n.nethercote)
Thanks for running DMD. I'm glad it worked :)

I've recently seen Mutex show up as using non-trivial amounts of memory in nsHostResolver, too. Two options:

1. ignore it, and write a comment about it being significant but not-yet-measured=

2. write a reporter for Mutex, in another bug that blocks this one

If you could do the latter, that would be great, but I won't insist because I don't want to impede progress in this bug.

As for nsIFile, there is an nsISizeOf interface (defined in xpcom/base/nsISizeOf.h) which you could use -- see the existing uses by searching for |nsISizeOf| in the code. Whether this is worth the effort depends on how much memory is not being reported.
Flags: needinfo?(n.nethercote)
Attached patch v2 (obsolete) — Splinter Review
I think this one is as complete as now possible.  I will handle the Mutex/Monitor sizeOf implementation later in a followup bug.

Nicholas, according XXX, Michal has sent you an email about it.
Attachment #8374443 - Attachment is obsolete: true
Attachment #8379686 - Flags: review?(n.nethercote)
Attachment #8379686 - Flags: review?(michal.novotny)
Blocks: 975367
About the XXX comment: line 460 of nsTHashtable.h looks like this:

  new(entry) EntryType(reinterpret_cast<KeyTypePointer>(key));

That's a "placement new" (http://en.wikipedia.org/wiki/Placement_syntax) which constructs an EntryType at the address given by |entry| without actually allocating new memory. And |entry| is the address of an element in the hash table's entry storage. So using key->SizeOfExcludingThis() is the right thing to do. The measurement of the entryStorage array is done by nsTHashtable::SizeOfExcludingThis().

I'll review the rest of the patch on Monday.
Duplicate of this bug: 940749
Comment on attachment 8379686 [details] [diff] [review]
v2

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

r=me, but the runnable stuff makes me nervous...

::: netwerk/cache2/CacheEntry.cpp
@@ +1559,5 @@
> +
> +size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  size_t n = 0;
> +  nsCOMPtr<nsISizeOf> sizeOf;

Move the declaration lower, to the point of its first use.

@@ +1563,5 @@
> +  nsCOMPtr<nsISizeOf> sizeOf;
> +
> +  n += mCallbacks.SizeOfExcludingThis(mallocSizeOf);
> +  if (mFile)
> +    n += mFile->SizeOfIncludingThis(mallocSizeOf);

Nit: brace all if blocks.

@@ +1567,5 @@
> +    n += mFile->SizeOfIncludingThis(mallocSizeOf);
> +
> +  sizeOf = do_QueryInterface(mURI);
> +  if (sizeOf)
> +    n += sizeOf->SizeOfIncludingThis(mallocSizeOf);

Ditto.

@@ +1576,5 @@
> +  // mDoomCallback is an arbitrary class that is probably reported elsewhere.
> +  // mOutputStream is reported in mFile.
> +  // mWriter is one of many handles we create, but (intentionally) not keep
> +  // any reference to, so those unfortunatelly cannot be reported.  Handles are
> +  // small, tho.

s/tho/though/

@@ +1585,5 @@
> +
> +size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  return mallocSizeOf(this) + SizeOfExcludingThis(mallocSizeOf);
> +}

Is this function used? If not, just remove it. Likewise elsewhere; we don't always need both SizeOfIncludingThis() and SizeOfExcludingThis().

::: netwerk/cache2/CacheFile.cpp
@@ +1558,5 @@
> +
> +namespace { // anon
> +
> +size_t
> +CollectChunkSize(uint32_t const & aIdx,

I'd remove the anon namespace and just make it static.

@@ +1576,5 @@
> +  size_t n = 0;
> +  n += mChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf);
> +  n += mCachedChunks.SizeOfExcludingThis(CollectChunkSize, mallocSizeOf);
> +  if (mMetadata)
> +    n += mMetadata->SizeOfIncludingThis(mallocSizeOf);

Brace all ifs. (There are lots more like this; I'll leave it up to you to find the rest.)

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +191,5 @@
> +{
> +  size_t n = 0;
> +  nsCOMPtr<nsISizeOf> sizeOf;
> +
> +  sizeOf = do_QueryInterface(mFile);

Combine |sizeOf| declaration and assignment.

@@ +488,5 @@
> +{
> +  // XXX SizeOfIncludingThis crashes since the new operator used at
> +  // nsTHashtable to allocate the HandleHashKey object is not overloaded
> +  // by the DMD core and thus not monitored - jemalloc goes crazy.
> +  return key->SizeOfExcludingThis(mallocSizeOf);

As discussed, SizeOfExcludingThis() is the right thing to use here.

@@ +3245,5 @@
> +    nsresult rv = target->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
> +    if (NS_FAILED(rv)) {
> +      NS_ERROR("Dispatch failed, cannot do memory report of CacheFileHandles");
> +      return 0;
> +    }

So you dispatch the runnable and then just wait for it to finish? That's a bit scary. The web worker reporters have to do some complicated stuff with mutexes and it took numerous attempts to get it working reliably.

Why is this necessary? Memory reporters always run on the main thread -- is that different to the I/O thread?

@@ +3279,5 @@
> +size_t
> +CacheFileIOManager::SizeOfExcludingThisInternal(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  size_t n = 0;
> +  nsCOMPtr<nsISizeOf> sizeOf;

Declare at first use.

@@ +3295,5 @@
> +  // mHandlesByLastUsed just refers handles reported by mHandles.
> +
> +  sizeOf = do_QueryInterface(mCacheDirectory);
> +  if (sizeOf)
> +    n += sizeOf->SizeOfIncludingThis(mallocSizeOf);

Do the various types you QI in this function actually implement nsISizeOf?

@@ +3305,5 @@
> +  sizeOf = do_QueryInterface(mTrashTimer);
> +  if (sizeOf)
> +    n += sizeOf->SizeOfIncludingThis(mallocSizeOf);
> +
> +  sizeOf = do_QueryInterface(mTrashDir);

I can't find where mTrashTimer or mTrashdir or mFailedTrashDirs are defined in mozilla-central...

@@ +3317,5 @@
> +  return n;
> +}
> +
> +// static
> +size_t

|/* static */ size_t| is more compact.

@@ +3323,5 @@
> +{
> +  if (!gInstance)
> +    return 0;
> +
> +  return gInstance->SizeOfExcludingThisInternal(mallocSizeOf);

You could use ?: here to make this more compact.

::: netwerk/cache2/CacheFileIOManager.h
@@ +277,5 @@
>    static void GetCacheDirectory(nsIFile** result);
>  
> +  // Memory reporting
> +  static size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
> +  static size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);

Are these two deliberately non-const? It's ok if they are, sometimes you have to do it.

::: netwerk/cache2/CacheFileInputStream.cpp
@@ +634,5 @@
> +
> +size_t
> +CacheFileInputStream::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  // Everything the stream referes is already reported somewhere else.

"referes"? Not sure what you meant by that.

::: netwerk/cache2/CacheFileOutputStream.cpp
@@ +395,5 @@
> +
> +size_t
> +CacheFileOutputStream::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +  // Everything the stream referes is already reported somewhere else.

"referes" again.

::: netwerk/cache2/CacheIndex.cpp
@@ +3279,5 @@
> +{
> +  // XXX SizeOfIncludingThis crashes since the new operator used at
> +  // nsTHashtable to allocate the CacheIndexEntry object is not overloaded
> +  // by the DMD core and thus not monitored - jemalloc goes crazy.
> +  return aEntry->SizeOfExcludingThis(mallocSizeOf);

Again, SizeOfExcludingThis() is appropriate here.

::: netwerk/cache2/CacheIndex.h
@@ +531,5 @@
>    static nsresult GetCacheSize(uint32_t *_retval);
>  
> +  // Memory reporting
> +  static size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
> +  static size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);

|const|?

::: netwerk/cache2/CacheStorageService.cpp
@@ +1479,5 @@
> +{
> +  CacheStorageService::Self()->Lock().AssertCurrentThreadOwns();
> +
> +  size_t n = 0;
> +  // The elemets are referenced by sGlobalEntryTables and are reported from there

s/elemets/elements/

@@ +1481,5 @@
> +
> +  size_t n = 0;
> +  // The elemets are referenced by sGlobalEntryTables and are reported from there
> +  n += mFrecencyArray.SizeOfExcludingThis(mallocSizeOf);
> +  // The elemets are referenced by sGlobalEntryTables and are reported from there

Ditto.

@@ +1483,5 @@
> +  // The elemets are referenced by sGlobalEntryTables and are reported from there
> +  n += mFrecencyArray.SizeOfExcludingThis(mallocSizeOf);
> +  // The elemets are referenced by sGlobalEntryTables and are reported from there
> +  n += mExpirationArray.SizeOfExcludingThis(mallocSizeOf);
> +  // Entries reported manually in CacheStorageService::CollectReports callback

Periods at the ends of these comments would be nice :)
(In reply to Nicholas Nethercote [:njn] from comment #16)
> Comment on attachment 8379686 [details] [diff] [review]
> v2
> 
> Review of attachment 8379686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but the runnable stuff makes me nervous...

Thanks.  See bellow.

> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +1559,5 @@
> > +
> > +size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  size_t n = 0;
> > +  nsCOMPtr<nsISizeOf> sizeOf;
> 
> Move the declaration lower, to the point of its first use.

Another style guide I really don't like... but I'll do it.

> @@ +1585,5 @@
> > +
> > +size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  return mallocSizeOf(this) + SizeOfExcludingThis(mallocSizeOf);
> > +}
> 
> Is this function used? If not, just remove it. Likewise elsewhere; we don't
> always need both SizeOfIncludingThis() and SizeOfExcludingThis().

Will remove.

> @@ +488,5 @@
> > +{
> > +  // XXX SizeOfIncludingThis crashes since the new operator used at
> > +  // nsTHashtable to allocate the HandleHashKey object is not overloaded
> > +  // by the DMD core and thus not monitored - jemalloc goes crazy.
> > +  return key->SizeOfExcludingThis(mallocSizeOf);
> 
> As discussed, SizeOfExcludingThis() is the right thing to use here.

OK, so it's just about to remove the comment?

> 
> @@ +3245,5 @@
> > +    nsresult rv = target->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
> > +    if (NS_FAILED(rv)) {
> > +      NS_ERROR("Dispatch failed, cannot do memory report of CacheFileHandles");
> > +      return 0;
> > +    }
> 
> So you dispatch the runnable and then just wait for it to finish? That's a
> bit scary. The web worker reporters have to do some complicated stuff with
> mutexes and it took numerous attempts to get it working reliably.

We (in networking!) do this all the time :)  and we know how to do it.  This is very simple code I really don't believe there is anything to worry about it.

> 
> Why is this necessary? Memory reporters always run on the main thread -- is
> that different to the I/O thread?

Probably the comment is not that good: the thing is that the access to the hashtable of open CacheFileHandle instances is designed to be only and only accessed on the I/O thread.  This is our design that solves a lot of problems and simplifies the code for us - there is then no locking.  So, when someone runs the memory report on the main thread while we alter the hashtable on the IO thread, this may crash, or even break the hashtable.  Not good.

> @@ +3295,5 @@
> > +  // mHandlesByLastUsed just refers handles reported by mHandles.
> > +
> > +  sizeOf = do_QueryInterface(mCacheDirectory);
> > +  if (sizeOf)
> > +    n += sizeOf->SizeOfIncludingThis(mallocSizeOf);
> 
> Do the various types you QI in this function actually implement nsISizeOf?

Actually, some of them may not.  At least I haven't seen them reported despite this QI->SizeOf..().  I left it in, so that when it's implemented, we get the data.  Here is no need to have the code here super optimized - this is no production code.

If you are against, I'll remove it.

> 
> @@ +3305,5 @@
> > +  sizeOf = do_QueryInterface(mTrashTimer);
> > +  if (sizeOf)
> > +    n += sizeOf->SizeOfIncludingThis(mallocSizeOf);
> > +
> > +  sizeOf = do_QueryInterface(mTrashDir);
> 
> I can't find where mTrashTimer or mTrashdir or mFailedTrashDirs are defined
> in mozilla-central...

Look at https://hg.mozilla.org/projects/gum/rev/tip, this is based on patches not yet landed (will be landed probably today).

> > +  // Memory reporting
> > +  static size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
> > +  static size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
> 
> Are these two deliberately non-const? It's ok if they are, sometimes you
> have to do it.

These two are deliberately static :)

> 
> ::: netwerk/cache2/CacheFileInputStream.cpp
> @@ +634,5 @@
> > +
> > +size_t
> > +CacheFileInputStream::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> > +{
> > +  // Everything the stream referes is already reported somewhere else.
> 
> "referes"? Not sure what you meant by that.

"Refers" is the word (typo).  Makes sense?

> ::: netwerk/cache2/CacheIndex.h
> @@ +531,5 @@
> >    static nsresult GetCacheSize(uint32_t *_retval);
> >  
> > +  // Memory reporting
> > +  static size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
> > +  static size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
> 
> |const|?

|static| ! :)
> > > +  // XXX SizeOfIncludingThis crashes since the new operator used at
> > > +  // nsTHashtable to allocate the HandleHashKey object is not overloaded
> > > +  // by the DMD core and thus not monitored - jemalloc goes crazy.
> > > +  return key->SizeOfExcludingThis(mallocSizeOf);
> > 
> > As discussed, SizeOfExcludingThis() is the right thing to use here.
> 
> OK, so it's just about to remove the comment?

Yeah. No point having a comment that effectively says "XXX changing this code to wrong code causes bad things to happen" :)

> > So you dispatch the runnable and then just wait for it to finish? That's a
> > bit scary. The web worker reporters have to do some complicated stuff with
> > mutexes and it took numerous attempts to get it working reliably.
> 
> We (in networking!) do this all the time :)  and we know how to do it.  This
> is very simple code I really don't believe there is anything to worry about
> it.

Ok, I'll take your word for it.

> > Do the various types you QI in this function actually implement nsISizeOf?
> 
> Actually, some of them may not.  At least I haven't seen them reported
> despite this QI->SizeOf..().  I left it in, so that when it's implemented,
> we get the data.  Here is no need to have the code here super optimized -
> this is no production code.
> 
> If you are against, I'll remove it.

No need to remove.

> > Are these two deliberately non-const? It's ok if they are, sometimes you
> > have to do it.
> 
> These two are deliberately static :)

Oh, right.
 
> "Refers" is the word (typo).  Makes sense?

Maybe "refers to" would be clearer?
(In reply to Nicholas Nethercote [:njn] from comment #18)
> Maybe "refers to" would be clearer?

"Keeps the reference to" could be the best I think.
Comment on attachment 8379686 [details] [diff] [review]
v2

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

Oh, did I forget to r+ this?
Attachment #8379686 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #20)
> Oh, did I forget to r+ this?

Thanks!
Attached patch v2.1 (obsolete) — Splinter Review
Some styling nits deliberately omitted - we don't enforce them strictly in necko code.
Attachment #8379686 - Flags: review?(michal.novotny)
Attachment #8382509 - Attachment description: v2.1 (backup) → v2.1
Attachment #8382509 - Flags: review?(michal.novotny)
Attachment #8379686 - Attachment is obsolete: true
Comment on attachment 8382509 [details] [diff] [review]
v2.1

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

r+ with few nits fixed

::: netwerk/cache2/CacheFileInputStream.cpp
@@ +638,5 @@
> +  // Everything the stream keeps a reference to is already reported somewhere else.
> +  // mFile reports itself.
> +  // mChunk reported as part of CacheFile.
> +  // mCallback is usually CacheFile or a class that is reported elsewhere.
> +  return 0;

We could move this comment to SizeOfIncludingThis() and don't define SizeOfExcludingThis() at all.

::: netwerk/cache2/CacheFileOutputStream.cpp
@@ +399,5 @@
> +  // Everything the stream keeps a reference to is already reported somewhere else.
> +  // mFile reports itself.
> +  // mChunk reported as part of CacheFile.
> +  // mCloseListener is CacheEntry, already reported.
> +  // mCallback is usually CacheFile or a class that is reported elsewhere.

The same case as CacheFileInputStream::SizeOfExcludingThis().

::: netwerk/cache2/CacheIOThread.cpp
@@ +260,5 @@
> +  size_t n = 0;
> +  n += mallocSizeOf(mThread);
> +  for (uint32_t level = 0; level < LAST_LEVEL; ++level) {
> +    n += mEventQueue[level].SizeOfExcludingThis(mallocSizeOf);
> +    // Objects held are arbitrary events we don't know are reported elsewhere.

This sentence doesn't make much sense to me.

::: netwerk/cache2/CacheIndex.cpp
@@ +3294,5 @@
> +  if (mIndexHandle) {
> +    n += mIndexHandle->SizeOfIncludingThis(mallocSizeOf);
> +  }
> +
> +  if (mJournalHandle) {

Handles are already reported in SizeOfHandlesRunnable::Run()

@@ +3306,5 @@
> +  // mFrecencyArray and mExpirationArray items are reported by
> +  // mIndex/mPendingUpdates
> +  n += mFrecencyArray.SizeOfExcludingThis(mallocSizeOf);
> +  n += mExpirationArray.SizeOfExcludingThis(mallocSizeOf);
> +

I think mCacheDirectory and mTimer should be reported too, right?
Attachment #8382509 - Flags: review?(michal.novotny) → review+
Attached patch v2.2 [to land]Splinter Review
Attachment #8382509 - Attachment is obsolete: true
Attachment #8385706 - Flags: review+
No idea how this should be automatically tested.
Flags: in-testsuite-
> No idea how this should be automatically tested.

Memory reporters are hard to test meaningfully, because they're not deterministic :( There is one mochitest that invokes all the reporters so at least if a reporter is crashy it'll be detected.
CacheFile::mKey is not reported.  I will update the patch.
https://hg.mozilla.org/mozilla-central/rev/a8cb3454d250
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 985156
(In reply to Honza Bambas (:mayhemer) from comment #28)
> CacheFile::mKey is not reported.  I will update the patch.

(bug 985156)
Depends on: 991669
Here's output from my nightly build:

├────8,931,392 B (01.01%) -- network
│    ├──8,435,776 B (00.96%) -- cache2
│    │  ├──6,437,472 B (00.73%) ── memory-storage(/M)
│    │  ├──1,398,960 B (00.16%) ── index
│    │  ├────565,360 B (00.06%) ── disk-storage()
│    │  ├─────12,976 B (00.00%) ── service
│    │  ├─────11,328 B (00.00%) ── disk-storage(a,)
│    │  └──────9,680 B (00.00%) ── io

The names for the *-storage entries are a bit weird: "/M", "", "a,". Is that expected?
Flags: needinfo?(honzab.moz)
Also, are those names privacy-sensitive? That's relevant for bug 1010064. Thanks!
(In reply to Nicholas Nethercote [:njn] from comment #32)
> Here's output from my nightly build:
> 
> ├────8,931,392 B (01.01%) -- network
> │    ├──8,435,776 B (00.96%) -- cache2
> │    │  ├──6,437,472 B (00.73%) ── memory-storage(/M)
> │    │  ├──1,398,960 B (00.16%) ── index
> │    │  ├────565,360 B (00.06%) ── disk-storage()
> │    │  ├─────12,976 B (00.00%) ── service
> │    │  ├─────11,328 B (00.00%) ── disk-storage(a,)
> │    │  └──────9,680 B (00.00%) ── io
> 
> The names for the *-storage entries are a bit weird: "/M", "", "a,". Is that
> expected?

Yes.

(In reply to Nicholas Nethercote [:njn] from comment #33)
> Also, are those names privacy-sensitive? That's relevant for bug 1010064.
> Thanks!

No, at least now.  The string is the scope key described here: https://developer.mozilla.org/en-US/docs/HTTP_Cache#Storage_and_entries_scopes

When it changes, I will take care not to expose any private data here!
Flags: needinfo?(honzab.moz)
Duplicate of this bug: 923255
You need to log in before you can comment on or make changes to this bug.