Last Comment Bug 699409 - Add telemetry for number of entries in nsDiskCacheMap
: Add telemetry for number of entries in nsDiskCacheMap
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 699951
  Show dependency treegraph
 
Reported: 2011-11-03 08:03 PDT by Nicholas Hurley [:nwgh][:hurley]
Modified: 2011-11-08 07:08 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.72 KB, patch)
2011-11-04 13:33 PDT, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review-
Details | Diff | Splinter Review
patch (2.91 KB, patch)
2011-11-07 10:30 PST, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review+
Details | Diff | Splinter Review
patch (3.04 KB, patch)
2011-11-07 13:36 PST, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review

Description Nicholas Hurley [:nwgh][:hurley] 2011-11-03 08:03:38 PDT
We would like to be able to estimate the memory overhead of the disk cache (specifically for mobile), so we would like to know how many entries (used AND unused) are in the map to do that calculation. We can probably just take a snapshot of this value at the time we open the disk cache.
Comment 1 Nicholas Hurley [:nwgh][:hurley] 2011-11-04 13:33:52 PDT
Created attachment 572060 [details] [diff] [review]
patch
Comment 2 Jason Duell [:jduell] (needinfo me) 2011-11-04 15:36:00 PDT
Comment on attachment 572060 [details] [diff] [review]
patch

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

::: netwerk/cache/nsDiskCacheMap.cpp
@@ +166,5 @@
>      rv = FlushHeader();
>      if (NS_FAILED(rv))  goto error_exit;
>      
> +    mozilla::Telemetry::Accumulate(mozilla::Telemetry::HTTP_DISK_CACHE_RECORDS,
> +            mHeader.mRecordCount);

I'd prefer to save the stat as the total amount of RAM used by the disk cache--that's the bottom line that most viewers will be interested in, not an entry count that sends them poking into the code to find out how much RAM that involves.

(Are there any other significant RAM uses by the disk cache, or is mRecordCount * recordSize the main culprit?)

We should also provide an about:memory reporter for the amount of RAM that the disk cache uses.  See bug 669117 for how to do this.  I'm thinking we could have that reporter and the telemetry use a single function that provides total disk cache RAM usage (like nsCacheService::MemoryDeviceSize() provides for the memory cache) on demand--telemetry would use it at :Open, and about:memory can use it whenever.

If that's too much work and you want to get the telemetry stuff in before the code fork, that's ok--file a followup for the about:memory work.
Comment 3 Nicholas Hurley [:nwgh][:hurley] 2011-11-04 15:43:42 PDT
(In reply to Jason Duell (:jduell) from comment #2)
Memory reporter is a great idea, and I'll gladly file a follow-up bug, but I'd like it if this lands soon to make it into the next Aurora so we can see the overhead at least from this on mobile.

mRecordCount * recordSize is the main culprit of memory use on a cache where there are no entries in use, and we have bug 699410 for telemetry on memory use by open entries.
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2011-11-04 15:46:46 PDT
followup filed - bug 699951
Comment 5 Nicholas Nethercote [:njn] 2011-11-05 12:43:28 PDT
Can you please please please use moz_malloc_usable_size(), so that any slop bytes caused by the heap allocator rounding up are included?  The idiom I've been using is this:

  size_t usable = moz_malloc_usable_size(p);
  size_t size = usable ? usable : mRecordCount * recordSize;

I have bug 698968 open to make this idiom nicer but that'll do for the moment.
Comment 6 Nicholas Hurley [:nwgh][:hurley] 2011-11-07 10:30:24 PST
Created attachment 572521 [details] [diff] [review]
patch

Update for actual memory usage of the records instead of a count
Comment 7 Jason Duell [:jduell] (needinfo me) 2011-11-07 10:54:16 PST
Comment on attachment 572521 [details] [diff] [review]
patch

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

::: netwerk/cache/nsDiskCacheMap.cpp
@@ +170,5 @@
> +        PRUint32 overhead = moz_malloc_usable_size(mRecordArray);
> +        overhead = overhead ? overhead : mHeader.mRecordCount * sizeof(nsDiskCacheRecord);
> +        mozilla::Telemetry::Accumulate(mozilla::Telemetry::HTTP_DISK_CACHE_OVERHEAD,
> +                overhead);
> +    }

Is there a reason for the extra brace scope?   I can't see one.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +165,5 @@
>  HISTOGRAM(CACHE_DEVICE_SEARCH, 1, 100, 100, LINEAR, "Time to search cache (ms)")
>  HISTOGRAM(CACHE_MEMORY_SEARCH, 1, 100, 100, LINEAR, "Time to search memory cache (ms)")
>  HISTOGRAM(CACHE_DISK_SEARCH, 1, 100, 100, LINEAR, "Time to search disk cache (ms)")
>  HISTOGRAM(CACHE_OFFLINE_SEARCH, 1, 100, 100, LINEAR, "Time to search offline cache (ms)")
> +HISTOGRAM(HTTP_DISK_CACHE_OVERHEAD, 1, 32000000, 100, EXPONENTIAL, "HTTP Disk cache memory overhead (bytes)")

I was gonna say, "no one wants byte-granularity, use KB at least", but I see the SQLite stats are all in bytes, so I guess that's fine.
Comment 8 Nicholas Hurley [:nwgh][:hurley] 2011-11-07 11:02:37 PST
(In reply to Jason Duell (:jduell) from comment #7)
> Comment on attachment 572521 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 572521 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache/nsDiskCacheMap.cpp
> @@ +170,5 @@
> > +        PRUint32 overhead = moz_malloc_usable_size(mRecordArray);
> > +        overhead = overhead ? overhead : mHeader.mRecordCount * sizeof(nsDiskCacheRecord);
> > +        mozilla::Telemetry::Accumulate(mozilla::Telemetry::HTTP_DISK_CACHE_OVERHEAD,
> > +                overhead);
> > +    }
> 
> Is there a reason for the extra brace scope?   I can't see one.
> 

The compiler barfed on some gotos in the same scope jumping past the declaration of "overhead", even though it's not used after the target of the gotos. New scope fixed that.
Comment 9 Jason Duell [:jduell] (needinfo me) 2011-11-07 11:07:08 PST
Ah, I'd never thought of that interaction with C++ scope and goto.  Ok, then. (maybe add a comment?  Or move decl of overhead higher up?  Whatever)
Comment 10 Nicholas Hurley [:nwgh][:hurley] 2011-11-07 13:36:39 PST
Created attachment 572597 [details] [diff] [review]
patch

Update w/comment about interaction between scoping & goto. Carry forward r+
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-11-07 20:42:39 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/5c0861ec147f
Comment 12 Ed Morley [:emorley] 2011-11-08 07:08:04 PST
https://hg.mozilla.org/mozilla-central/rev/5c0861ec147f

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