699409 - Add telemetry for number of entries in nsDiskCacheMap

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[u408661]

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

[u408661]

Attachment: patch

Comment 2

[Jason Duell]

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

[u408661]

(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

[u408661]

followup filed - bug 699951

Comment 5

[Nicholas Nethercote [inactive]]

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

[u408661]

Attachment: patch

Update for actual memory usage of the records instead of a count

Comment 7

[Jason Duell]

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

[u408661]

(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]

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

[u408661]

Attachment: patch

Update w/comment about interaction between scoping & goto. Carry forward r+

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c0861ec147f

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c0861ec147f