Try to precompute the bucket range

RESOLVED FIXED in Firefox 57



28 days ago
9 days ago


(Reporter: Ehsan, Assigned: dthayer)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)


(Whiteboard: [qf:p1])

MozReview Requests


Submitter Diff Changes Open Issues Last Updated
Error loading review requests:


(1 attachment)

base::Histogram::InitializeBucketRange() <> shows up in startup profiles due to the expensive log() and exp() calls there.

It would be nice for built-in telemetry probes to compute these bucket ranges at compile time and store them as static data if possible.
This shows up with 30ms inside InitializeBucketRange().
Priority: -- → P2
Whiteboard: [qf]
Depends on: 1366294


25 days ago
Whiteboard: [qf] → [qf:p1]

Comment 2

16 days ago
Unless someone was planning on taking this I can work on it.
Assignee: nobody → dothayer
Comment hidden (mozreview-request)

Comment 4

14 days ago
Also, I tested this by adding test code to create a histogram with the old method whenever we create a histogram, and asserting that all of their ranges were equal. But let me know if you have any further suggestions for how to test this.

Comment 5

10 days ago
Comment on attachment 8893893 [details]
Bug 1383210 - Use precomputed histogram buckets

Thanks for picking this up!
This looks good overall and the manual testing sounds like a reasonable approach.
I only have some smaller requests below.

::: ipc/chromium/src/base/histogram.h:325
(Diff revision 1)
>    // For each index, show the least value that can be stored in the
>    // corresponding bucket. We also append one extra element in this array,
>    // containing kSampleType_MAX, to make calculations easy.
>    // The dimension of ranges_ is bucket_count + 1.

Sanity-checking this; can we add `DCHECK(ValidateBucketRanges());` at the end of `InitializeBucketRangeFromData()`?
That would assert that this assumption holds.

::: ipc/chromium/src/base/
(Diff revision 1)
>    histogram = new Histogram(minimum, maximum, bucket_count);
> +  if (buckets) {
> +    histogram->InitializeBucketRangeFromData(buckets);
> +  } else {
> -  histogram->InitializeBucketRange();
> +    histogram->InitializeBucketRange();

I think we can make `buckets` a required argument here and assert that it is non-null.

There are only two callers for `Histogram::FactoryGet()`:

One is from TelemetryHistogram.cpp and has the precomputed buckets available.
The other is from `Histogram::FactoryTimeGet()`, which is not used anywhere, so we could just remove it.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:243
(Diff revision 1)
>  namespace {
>  // Factory function for histogram instances.
>  Histogram*
> -internal_CreateHistogramInstance(const HistogramInfo& info);
> +internal_CreateHistogramInstance(const HistogramInfo& info, int bucketsOffset = -1);

Can we make `bucketsOffset` required?
I'm not clear on where we would want to leave this out.

::: toolkit/components/telemetry/TelemetryHistogram.cpp:268
(Diff revision 1)
>    if (h || !instantiate) {
>      return h;
>    }
>    const HistogramInfo& info = gHistogramInfos[histogramId];
> -  h = internal_CreateHistogramInstance(info);
> +  const int bucketsOffset = gExponentialBucketLowerBoundIndex[histogramId];\

Trailing `\\` on this line.

::: toolkit/components/telemetry/
(Diff revision 1)
> +        if histogram.kind() == 'exponential':
> +            if cpp_guard:
> +                print("#if defined(%s)" % cpp_guard, file=output)
> +            print("%d," % offset, file=output)

Can we restructure this a bit to only do the printing logic once?
if histogram.kind() == 'exponential':
  value = offset
  offset += ...
Attachment #8893893 - Flags: review?(gfritzsche)
Comment hidden (mozreview-request)

Comment 7

10 days ago
Comment on attachment 8893893 [details]
Bug 1383210 - Use precomputed histogram buckets
Attachment #8893893 - Flags: review?(gfritzsche) → review+

Comment 8

10 days ago
Pushed by
Use precomputed histogram buckets r=gfritzsche
Backed out for flake8 failures like
Flags: needinfo?(dothayer)
Comment hidden (mozreview-request)

Comment 11

10 days ago
Pushed by
Use precomputed histogram buckets r=gfritzsche


10 days ago
Flags: needinfo?(dothayer)

Comment 12

10 days ago
Last Resolved: 10 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → bug 1388748
You need to log in before you can comment on or make changes to this bug.