Possible Overflows in DataUse.ContentType Histograms |
|||
Issue descriptionIn http://b/34077453#comment2 it was pointed out to me that the histograms in the changelist "Record the data use by content type" https://codereview.chromium.org/2595503002/ are likely to overflow. >>> It appears that you're incrementing each bucket by the number of bytes. ... Importantly, the "count" is limited at 32-bits even on 64-bit platforms. Since you're counting video bytes, it seems likely that you're going to overflow these counters which will cause errors to reported as the sum of the 32-bit counters fails to match the total 64-bit counter. >>> You should evaluate your use of this histogram to consider whether this can affect you. Apologies that I didn't catch this during the review.
,
Jan 6 2017
Thanks for pointing this out. I will look at moving the histograms to using STATIC_HISTOGRAM_POINTER_GROUP or some other technique. "Memory.PressureLevel" is another histogram that may exhibit this problem.
,
Jan 6 2017
> I will look at moving the histograms to using STATIC_HISTOGRAM_POINTER_GROUP > or some other technique. That will not help. That's just a way to write simpler code. I.e., instead of if (...) emit to "A" histogram else if (...) emit to "B" histogram else if (...) emit to "C" histogram, it will let you write that in one line a function GetHistogramNameForIndex(). It does not have do anything about the overflow problem.
,
Jan 6 2017
Ok. got it.
,
Apr 13 2017
The histogram counters are not reset after an UMA upload, and will live for the entire browser run. So overflows are highly likely. Overflows are seen in Desktop for DataUse.ContentType.UserTraffic histogram. The histograms should be converted to a 1kB increment, or should be logged as a regular UMA(instead of incrementing using AddCount).
,
Apr 30 2017
We shouldn't have overflows.
,
May 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c9c9ed06b946f1e9666212df891ffe8fc873077 commit 6c9c9ed06b946f1e9666212df891ffe8fc873077 Author: rajendrant <rajendrant@chromium.org> Date: Thu May 04 19:21:39 2017 Fix overflow in user traffic content type histogram Overflows are likely for DataUse.ContentType.UserTraffic histogram. Fix the overflow by logging the histogram in increments of kilobytes. BUG= 679046 Review-Url: https://codereview.chromium.org/2851923002 Cr-Commit-Position: refs/heads/master@{#469419} [modify] https://crrev.com/6c9c9ed06b946f1e9666212df891ffe8fc873077/components/data_use_measurement/core/data_use_measurement.cc [modify] https://crrev.com/6c9c9ed06b946f1e9666212df891ffe8fc873077/components/data_use_measurement/core/data_use_measurement.h [modify] https://crrev.com/6c9c9ed06b946f1e9666212df891ffe8fc873077/components/data_use_measurement/core/data_use_measurement_unittest.cc [modify] https://crrev.com/6c9c9ed06b946f1e9666212df891ffe8fc873077/tools/metrics/histograms/histograms.xml
,
May 4 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by mpear...@chromium.org
, Jan 6 2017