Last Comment Bug 699951 - Add a memory reporter for Necko's disk cache
: Add a memory reporter for Necko's disk cache
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla19
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
Mentors:
Depends on: 699409
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-11-04 15:46 PDT by Nicholas Hurley [:nwgh][:hurley]
Modified: 2012-11-06 06:13 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.72 KB, patch)
2011-11-08 13:36 PST, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review+
justin.lebar+bug: review-
Details | Diff | Splinter Review
patch (14.88 KB, patch)
2012-10-24 10:26 PDT, Nicholas Hurley [:nwgh][:hurley]
michal.novotny: review-
n.nethercote: feedback+
Details | Diff | Splinter Review
patch (16.28 KB, patch)
2012-10-30 14:38 PDT, Nicholas Hurley [:nwgh][:hurley]
michal.novotny: review+
Details | Diff | Splinter Review

Description Nicholas Hurley [:nwgh][:hurley] 2011-11-04 15:46:08 PDT
+++ This bug was initially created as a clone of Bug #699409 +++

(from bug 699409 comment 2):
We should also provide an about:memory reporter for the amount of RAM that the disk cache uses.
Comment 1 Jason Duell [:jduell] (needinfo me) 2011-11-04 15:58:44 PDT
Note that we may want to tree-ify the two necko cache about:memory entries (see bug 669117 comment 14):  ask Nicolas how to to that, or spin off into separate bug if he agrees it's worth doing.

I also just noticed that I don't see network-memory-cache in about:memory--not sure why that is--commented in original bug.
Comment 2 Nicholas Nethercote [:njn] 2011-11-05 12:49:18 PDT
Tree-ifying is dead easy -- memory reporter paths have the form "a/b/c" and the '/' chars denote the tree structure.  So if you change "network-memory-cache" to "network/memory-cache" and add "network/disk-cache", you'll end up with this structure:

- network
  - memory-cache
  - disk-cache

Just in case I haven't made the point clear enough, please use moz_malloc_usable_size() for this if you can :)

BTW, any idea how big is this likely to be?  And does it grow over time?
Comment 3 Justin Lebar (not reading bugmail) 2011-11-08 07:01:46 PST
Ping re

> BTW, any idea how big is this likely to be?

This would help us triage this bug in today's MemShrink meeting.
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2011-11-08 11:34:29 PST
(In reply to Justin Lebar [:jlebar] from comment #3)
> Ping re
> 
> > BTW, any idea how big is this likely to be?
> 
> This would help us triage this bug in today's MemShrink meeting.

Running on a local build with ~7-10 tabs open, we seem to be using a little less than 300k of heap in the disk cache (the disk cache has about 8k entries total... my usual browser has between 20k and 40k entries, which would increase the memory usage, don't know by how much yet, though... hence this patch)
Comment 5 Nicholas Hurley [:nwgh][:hurley] 2011-11-08 13:36:13 PST
Created attachment 572991 [details] [diff] [review]
patch

Adds disk cache memory reporter + makes network into a hierarchy, so memory-cache and disk-cache appear together.
Comment 6 Nicholas Nethercote [:njn] 2011-11-08 16:54:05 PST
Thanks for using moz_malloc_usable_size.  Bug 698968 will simplify the |usable ? usable : computeSize()| idiom.
Comment 7 Jason Duell [:jduell] (needinfo me) 2011-11-27 17:53:00 PST
Comment on attachment 572991 [details] [diff] [review]
patch

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

Looks fine.  I'm not the person to know if you might have missed any disk cache data structures that could also be counted.  If you're confident you know all the data structures involved, go ahead and land.  Otherwise ask Michal or Bjarne to look over this too.
Comment 8 Nicholas Hurley [:nwgh][:hurley] 2011-11-28 09:51:52 PST
Comment on attachment 572991 [details] [diff] [review]
patch

Just want to make sure I didn't miss any other data structures to count
Comment 9 Bjarne (:bjarne) 2011-11-28 16:10:13 PST
Comment on attachment 572991 [details] [diff] [review]
patch

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

Afaics, you sum up total heap-usage like this

size(disk-device) = size(mCacheMap) + size(mBindery)

size(mCacheMap) = size(mRecordArray) + size(all(mBlockFile))
size(mBindery) = size(all(binding)) + size(all(mCacheEntry)) + size(all(mStreamInfo))

size(mBlockFile) = size of the mBitMap
size(mStreamInfo) = size(mOutputStream) + size(mBuffer)

Since I'm not familiar with details of the memory-reporting mechanism, answers to my issues may be obvious but I'll post them anyway... also requesting comments from Michal who has a lot more mileage with the disk-cache than myself.

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +408,5 @@
> +        *usage += sz ? sz : sizeof(*binding);
> +
> +        if (binding->mCacheEntry) {
> +            sz = moz_malloc_usable_size(binding->mCacheEntry);
> +            *usage += sz ? sz : sizeof(*(binding->mCacheEntry));

Isn't this accounted for somewhere else? The nsCacheEntry is not specific to the disk-cache. However, if you really want to count it here, it has fields like mKey and mSecurityInfo which may have to be counted as well...

@@ +416,5 @@
> +            sz = moz_malloc_usable_size(binding->mStreamIO);
> +            *usage += sz ? sz : sizeof(*(binding->mStreamIO));
> +            *usage += binding->mStreamIO->HeapUsage();
> +        }
> +

Why not mDeactivateEvent?

::: netwerk/cache/nsDiskCacheBlockFile.cpp
@@ +409,5 @@
> +PRInt64
> +nsDiskCacheBlockFile::HeapUsage()
> +{
> +  PRInt64 usage = moz_malloc_usable_size(mBitMap);
> +  return usage ? usage : (PRInt64)mBitMapWords * 4;

Why not mFD?

::: netwerk/cache/nsDiskCacheMap.cpp
@@ +1161,5 @@
> +  usage = usage ? usage : mHeader.mRecordCount * sizeof(nsDiskCacheRecord);
> +
> +  for (int i = 0; i < kNumBlockFiles; i++) {
> +    usage += mBlockFile[i].HeapUsage();
> +  }

Why not mMapFD or mBuffer?

::: netwerk/cache/nsDiskCacheStreams.cpp
@@ +958,5 @@
> +    if (mBuffer) {
> +        sz = moz_malloc_usable_size(mBuffer);
> +        usage += sz ? sz : mBufSize;
> +    }
> +

Why not mFD?
Comment 10 Justin Lebar (not reading bugmail) 2011-11-28 16:33:24 PST
Comment on attachment 572991 [details] [diff] [review]
patch

Hm.  The new convention (as of bug 698968) is functions named SizeOfIncludingThis, SizeOfExcludingThis, or SizeOfXXX when you're measuring something else.  Is that going to be too confusing in this case?

These functions need to take an nsMallocSizeFun parameter, whatever they're named.  See https://wiki.mozilla.org/Memory_Reporting

+PRInt64
+nsDiskCacheBindery::HeapUsage()
+{
+    NS_ASSERTION(initialized, "nsDiskCacheBindery not initialized");
+    if (!initialized) return 0;
+
+    PRInt64 usage = 0;
+    PL_DHashTableEnumerate(&table, AccumulateHeapUsage, &usage);
+
+    return usage;
+}

Should count the size of the PLDHashTable itself, as well, unless it's always very small.
Comment 11 Nicholas Nethercote [:njn] 2011-11-28 16:44:44 PST
jduell, I recently landed bug 698968 which introduces some stronger conventions for memory reporters that I'd like you to follow here.  They are described in https://wiki.mozilla.org/Memory_Reporting.  The two main things that apply to your patch:

- Functions that measure heap memory usage should be called SizeOfIncludingThis() or SizeOfExcludingThis(), depending on whether or not they measure the object itself.

- You should pass in an nsMallocSizeOfFun pointer to all your SizeOf* functions and use it instead of moz_malloc_usable_size.  You should use the following code to create a specific nsMallocSizeOfFun function:

  NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(NetworkDiskCacheMallocSizeOf, "network/disk-cache")

That'll create a function called |NetworkDiskCacheMallocSizeOf| which can be passed in at the top, IYSWIM.

I'm sorry that I shifted things under your feet like this, but if you don't make these changes now I'll just have to go and make them after the patch has landed, so you might as well do them now :)
Comment 12 Nicholas Hurley [:nwgh][:hurley] 2011-11-28 17:18:52 PST
> I'm sorry that I shifted things under your feet like this, but if you don't
> make these changes now I'll just have to go and make them after the patch
> has landed, so you might as well do them now :)

No worries, after a quick skim it all seems straightforward enough. New patch is in the works!
Comment 13 Michal Novotny (:michal) 2011-11-29 06:41:30 PST
Comment on attachment 572991 [details] [diff] [review]
patch

(In reply to Bjarne (:bjarne) from comment #9)
> Since I'm not familiar with details of the memory-reporting mechanism,
> answers to my issues may be obvious but I'll post them anyway... also
> requesting comments from Michal who has a lot more mileage with the
> disk-cache than myself.

I find all your comments relevant.


> +    if (mOutStream) {
> +        sz = moz_malloc_usable_size(mOutStream);
> +        usage += sz ? sz : sizeof(*mOutStream);
> +    }

Hmm, we also allocate nsDiskCacheInputStream, but we don't keep the pointer, so there is probably nothing we can do about it.
Comment 14 Nicholas Nethercote [:njn] 2012-04-17 23:18:27 PDT
Ping?
Comment 15 Nicholas Hurley [:nwgh][:hurley] 2012-10-24 10:26:46 PDT
Created attachment 674725 [details] [diff] [review]
patch

Would you look at that, an updated patch! All original review comments addressed, try looks good https://tbpl.mozilla.org/?tree=Try&rev=bad1961d807e
Comment 16 Nicholas Nethercote [:njn] 2012-10-24 12:45:55 PDT
Comment on attachment 674725 [details] [diff] [review]
patch

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

Thanks for getting back to this.

::: netwerk/cache/nsCacheService.cpp
@@ +1081,5 @@
>      UNITS_BYTES,
>      nsCacheService::MemoryDeviceSize,
>      "Memory used by the network memory cache.")
>  
> +NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(NetworkDiskCacheSizeOfFun, "network disk cache")

Nit: convention is to use dashes, i.e "network-disk-cache".

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +370,5 @@
>  }
> +
> +struct AccumulatorArg {
> +    size_t usage;
> +    nsMallocSizeOfFun mallocSizeOf;

|mUsage| and |mMallocSizeOf|?

@@ +390,5 @@
> +        if (binding->mStreamIO) {
> +            acc->usage += binding->mStreamIO->SizeOfIncludingThis(acc->mallocSizeOf);
> +        }
> +
> +        /* XXX - no good way to get at mDeactivateEvent here for proper size */

I don't understand this comment.

::: netwerk/cache/nsDiskCacheMap.cpp
@@ +1227,5 @@
> +  for (int i = 0; i < kNumBlockFiles; i++) {
> +    usage += mBlockFile[i].SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  return usage;

Uber-nit: you've used |usage| here and various other places, but |n| is usually used elsewhere.
Comment 17 Michal Novotny (:michal) 2012-10-30 05:07:48 PDT
Comment on attachment 674725 [details] [diff] [review]
patch

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

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +375,5 @@
> +};
> +
> +PLDHashOperator
> +AccumulateHeapUsage(PLDHashTable *table, PLDHashEntryHdr *hdr, uint32_t number,
> +                    void *arg)

I think we need to grab the cache lock in nsCacheService::DiskDeviceHeapSize(), otherwise we could operate on bindery on the other thread while enumerating it here.
Comment 18 Nicholas Hurley [:nwgh][:hurley] 2012-10-30 14:38:44 PDT
Created attachment 676783 [details] [diff] [review]
patch

Total facepalm on not acquiring the lock. This fixes that, as well as njn's nits (with the exception of his uber-nit about usage vs. n, I'm too lazy to change that :)
Comment 19 Nicholas Hurley [:nwgh][:hurley] 2012-11-05 10:23:13 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c87fcf6ed0a7
Comment 20 Ed Morley [:emorley] 2012-11-06 06:13:54 PST
https://hg.mozilla.org/mozilla-central/rev/c87fcf6ed0a7

Note You need to log in before you can comment on or make changes to this bug.