Try to precompute the bucket range

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Telemetry
P2
normal
RESOLVED FIXED
28 days ago
9 days ago

People

(Reporter: Ehsan, Assigned: dthayer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

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

Attachments

(1 attachment)

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.
This shows up with 30ms inside InitializeBucketRange().
Priority: -- → P2
Whiteboard: [qf]
Depends on: 1366294

Updated

25 days ago
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 2

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

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
mozreview-review
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 += ...
...
```
Attachment #8893893 - Flags: review?(gfritzsche)
Comment hidden (mozreview-request)

Comment 7

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

https://reviewboard.mozilla.org/r/164968/#review171290
Attachment #8893893 - Flags: review?(gfritzsche) → review+

Comment 8

10 days ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/277c7e4952a8
Use precomputed histogram buckets r=gfritzsche
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
Flags: needinfo?(dothayer)
Comment hidden (mozreview-request)

Comment 11

10 days ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8e78ebe648e
Use precomputed histogram buckets r=gfritzsche
(Assignee)

Updated

10 days ago
Flags: needinfo?(dothayer)

Comment 12

10 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8e78ebe648e
Status: ASSIGNED → RESOLVED
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.