Last Comment Bug 781200 - fixup several cache histograms
: fixup several cache histograms
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on:
Blocks: 763359 764585
  Show dependency treegraph
 
Reported: 2012-08-08 08:35 PDT by Nathan Froyd [:froydnj]
Modified: 2012-08-23 19:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.75 KB, patch)
2012-08-08 09:06 PDT, Nathan Froyd [:froydnj]
hurley: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-08-08 08:35:36 PDT
Several cache histograms aren't properly using the LINEAR format; this is blocking bug 764585 from making LINEAR histogram checking more stringent.
Comment 1 Nathan Froyd [:froydnj] 2012-08-08 09:06:50 PDT
Created attachment 650157 [details] [diff] [review]
patch

This patch changes several LINEAR histograms that didn't bucket properly into EXPONENTIAL histograms with bigger ranges.  The ranges are more in line with other cache histograms now; 100ms at the top end seemed rather restrictive anyway.

I also took the liberty of:

- Making DISK_CACHE_CORRUPT_DETAILS use HISTOGRAM_ENUMERATED_VALUES;
- Making CACHE_LM_INCONSISTENT use HISTOGRAM_BOOLEAN.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-08-22 15:11:07 PDT
Comment on attachment 650157 [details] [diff] [review]
patch

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

LGTM, but this will mean that it is difficult to compare this telemetry across this change, especially the lock wait telemetry. Please check with Nick to make sure that won't cause problems with the cache lock work he's working on.
Comment 3 Nicholas Hurley [:nwgh][:hurley] 2012-08-22 15:29:07 PDT
Comment on attachment 650157 [details] [diff] [review]
patch

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

Looks fine to me, with one (possible) issue below. r+ with that addressed (or ignored, as appropriate)

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +194,5 @@
>  
>  #undef _HTTP_HIST
>  #undef HTTP_HISTOGRAMS
>  
> +HISTOGRAM_ENUMERATED_VALUES(DISK_CACHE_CORRUPT_DETAILS, 50, "Why the HTTP disk cache was corrupted at startup")

Do we need to rename this DISK_CACHE_CORRUPT_DETAILS_2 like we did for the other ones that have already been converted to an enumerated values histogram? I'm not versed on the details of the translation between the two types.
Comment 4 Nathan Froyd [:froydnj] 2012-08-22 18:29:45 PDT
Brian, Nick, thanks for the reviews.

(In reply to Nick Hurley [:hurley] from comment #3)
> > +HISTOGRAM_ENUMERATED_VALUES(DISK_CACHE_CORRUPT_DETAILS, 50, "Why the HTTP disk cache was corrupted at startup")
> 
> Do we need to rename this DISK_CACHE_CORRUPT_DETAILS_2 like we did for the
> other ones that have already been converted to an enumerated values
> histogram?

We do not need to rename this one, since the current definition is:

HISTOGRAM(DISK_CACHE_CORRUPT_DETAILS, 1, 50, 51, LINEAR, "Why the HTTP disk cache was corrupted at startup")

and HISTOGRAM_ENUMERATED_VALUES is defined as:

#define HISTOGRAM_ENUMERATED_VALUES(id, n, message) \
  HISTOGRAM(id, 1, n, n+1, LINEAR, message)

and so the new definition of DISK_CACHE_CORRUPT_DETAILS expands to the same thing as the old definition.  It simply makes intent clearer.

(In reply to Brian Smith (:bsmith) from comment #2)
> LGTM, but this will mean that it is difficult to compare this telemetry
> across this change, especially the lock wait telemetry. Please check with
> Nick to make sure that won't cause problems with the cache lock work he's
> working on.

For avoidance of doubt, Nick, are you OK with the cache lock telemetry measurements changing out from underneath you?  (I don't know anything about your cache lock work or when it's projected to land; if it's not for another cycle or two, these changes may be beneficial because they'll give you better data than the current histograms.)
Comment 5 Nicholas Hurley [:nwgh][:hurley] 2012-08-23 09:45:50 PDT
Yes, I'm fine with the measurements changing out from underneath me. Better data is... better :)
Comment 6 Nathan Froyd [:froydnj] 2012-08-23 12:11:40 PDT
(In reply to Nick Hurley [:hurley] from comment #5)
> Yes, I'm fine with the measurements changing out from underneath me. Better
> data is... better :)

:)  Thanks for the confirmation.  I took the additional liberty of using HISTOGRAM_BOOLEAN for DISK_CACHE_SMART_SIZE_USING_OLD_MAX as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/042463c88673
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:17:25 PDT
https://hg.mozilla.org/mozilla-central/rev/042463c88673

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