Closed
Bug 1383210
Opened 6 years ago
Closed 6 years ago
Try to precompute the bucket range
Categories
(Toolkit :: Telemetry, enhancement, P2)
Toolkit
Telemetry
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: dthayer)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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•6 years ago
|
||
This shows up with 30ms inside InitializeBucketRange().
Priority: -- → P2
Whiteboard: [qf]
Updated•6 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 2•6 years 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•6 years 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•6 years 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•6 years 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+
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•6 years ago
|
||
Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8e78ebe648e Use precomputed histogram buckets r=gfritzsche
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dothayer)
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8e78ebe648e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•