1383210 - Try to precompute the bucket range

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P2)

Description

[(no longer active)]

base::Histogram::InitializeBucketRange() <https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/ipc/chromium/src/base/histogram.cc#387> shows up in startup profiles due to the expensive log() and exp() calls there.

https://perfht.ml/2tnSD90

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

This shows up with 30ms inside InitializeBucketRange().

Comment 2

[Doug Thayer [:dthayer] (he/him)]

Unless someone was planning on taking this I can work on it.

Comment 3

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1383210 - Use precomputed histogram buckets

The log and exp calls in base::Histogram::InitializeBucketRange()
were showing up in profiles. This patch uses the precomputed
buckets for exponential histograms instead of computing them at
runtime. Though linear histograms do show up in the profile that
prompted this change, they contribute much less, and due to the
trivial nature of generating these, it's unlikely that a static
cache would provide much if any speedup.

Review commit: https://reviewboard.mozilla.org/r/164968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164968/

Comment 4

[Doug Thayer [:dthayer] (he/him)]

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

[Georg Fritzsche [:gfritzsche]]

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

https://reviewboard.mozilla.org/r/164968/#review171208

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/histogram.cc:101
(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()`:
http://searchfox.org/mozilla-central/search?q=symbol:_ZN4base9Histogram10FactoryGetEiimNS0_5FlagsE&redirect=false

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/gen-histogram-data.py:155
(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 += ...
...
```

Comment 6

[Doug Thayer [:dthayer] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164968/diff/1-2/

Comment 7

[Georg Fritzsche [:gfritzsche]]

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

https://reviewboard.mozilla.org/r/164968/#review171290

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/277c7e4952a8
Use precomputed histogram buckets r=gfritzsche

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for flake8 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=121745940&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/aebbe078736848abc23b7d652acdcc65987aa71a

Comment 10

[Doug Thayer [:dthayer] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164968/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8e78ebe648e
Use precomputed histogram buckets r=gfritzsche

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8e78ebe648e