The default bug view has changed. See this FAQ.

fixup several cache histograms

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Several cache histograms aren't properly using the LINEAR format; this is blocking bug 764585 from making LINEAR histogram checking more stringent.
(Assignee)

Comment 1

5 years ago
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.
Attachment #650157 - Flags: review?(bsmith)
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.
Attachment #650157 - Flags: review?(bsmith) → review?(hurley)
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.
Attachment #650157 - Flags: review?(hurley) → review+
(Assignee)

Comment 4

5 years ago
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.)
Yes, I'm fine with the measurements changing out from underneath me. Better data is... better :)
(Assignee)

Comment 6

5 years ago
(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
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/042463c88673
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.