Closed Bug 1383210 Opened 7 years ago Closed 7 years ago

Try to precompute the bucket range


(Toolkit :: Telemetry, enhancement, P2)




Performance Impact high
Tracking Status
firefox57 --- fixed


(Reporter: ehsan.akhgari, Assigned: alexical)


(Blocks 1 open bug)



(1 file)

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
Whiteboard: [qf] → [qf:p1]
Unless someone was planning on taking this I can work on it.
Assignee: nobody → dothayer
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 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 on attachment 8893893 [details]
Bug 1383210 - Use precomputed histogram buckets
Attachment #8893893 - Flags: review?(gfritzsche) → review+
Pushed by
Use precomputed histogram buckets r=gfritzsche
Pushed by
Use precomputed histogram buckets r=gfritzsche
Flags: needinfo?(dothayer)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1388748
Depends on: 1397376
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.