Closed
Bug 1397376
Opened 7 years ago
Closed 7 years ago
gExponentialBucketLowerBounds is too big
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox56 | --- | unaffected |
firefox57 | + | fixed |
People
(Reporter: froydnj, Assigned: alexical)
References
Details
Attachments
(3 files)
gExponentialBucketLowerBounds is huge: it's 294K, the single largest object in libxul. Many of its entries are duplicated, wasting space. It looks like there's already a mechanism to point at the index of buckets for each histogram; could we please eliminate duplicate bucket lists and point multiple histograms at the same bucket list from gExponentialBucketLowerBounds to save some space? It looks like we might be able to save ~half the space, which is still huge and possibly unnecessary, but perhaps a little more tolerable.
Flags: needinfo?(dothayer)
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(dothayer)
Assignee | ||
Comment 1•7 years ago
|
||
Eliminating duplicate ranges brings the length of the array from about 76000 to about 12000. Also, 90% of the deduplicated values are under 65536, so we could encode them as uint16_t's, reducing the total size by a further 45% - but that might be overkill, and would impose a (probably trivial) runtime cost. Thoughts, Georg?
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Doug Thayer [:dthayer] from comment #1) > Eliminating duplicate ranges brings the length of the array from about 76000 > to about 12000. This is excellent, thank you Doug! I think it would be reasonable to have histogram code that's willing to expand uint16_t bucket ranges to the int ranges required by the rest of the code; Histogram::InitializeBucketRangesFromData already does a copy even with the current, precalculated bucket ranges. Doing a copy from uint16_t bucket ranges in some cases would not be substantially more costly. (Of course, Histogram's code could be rewritten to not require the extra copy, in which case the uint16_t compression might be less desirable.)
Reporter | ||
Updated•7 years ago
|
Summary: gExponentialBuckerLowerBounds is too big → gExponentialBucketLowerBounds is too big
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8905654 [details] Bug 1397376 - Deduplicate histogram static bucket data https://reviewboard.mozilla.org/r/177446/#review182780 ::: toolkit/components/telemetry/gen-histogram-data.py:170 (Diff revision 1) > for histogram in histograms: > if histogram.kind() == 'exponential': > - ranges = histogram.ranges() > + ranges = tuple(histogram.ranges()) > + if ranges not in ranges_offsets: > + ranges_offsets[ranges] = offset > + offset += histogram.n_buckets() We write out `ranges` and want to track the offset to that. Why not just use `+= len(ranges)` or so, am i missing something?
Attachment #8905654 -
Flags: review?(gfritzsche) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8905655 [details] Bug 1397376 - Reduce size of gExponentialBucketLowerBoundIndex https://reviewboard.mozilla.org/r/177448/#review182784
Attachment #8905655 -
Flags: review?(gfritzsche) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8905656 [details] Bug 1397376 - Avoid copying on initializing histogram ranges https://reviewboard.mozilla.org/r/177450/#review182810 ::: ipc/chromium/src/base/histogram.cc:277 (Diff revision 1) > - std::copy_n(buckets, bucket_count_, ranges_.data()); > + // buckets is a 16-bit encoded buffer of values that can go up to INT_MAX, > + // and which omits the 0 which we can assume to always be present. We can This is doing more than just copying values over and introduces potential runtime cost for initializing histograms. In bug 1383210 we specifically introduced the precomputed bucket ranges to avoid this. Are we sure this doesn't introduce any additional costs? I would rather eliminate the copies of the bucket ranges each histogram has. That reduces memory footprint and definitely has no performance impact. What do you think?
Attachment #8905656 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 9•7 years ago
|
||
Some bitshifting amidst copying--what the patch proposes--is hardly comparable to calling log and exp during bucket computation.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8905656 [details] Bug 1397376 - Avoid copying on initializing histogram ranges https://reviewboard.mozilla.org/r/177450/#review183384 This looks good to me, thanks! I'll flag Chris for a sanity pass as he has been working a lot with the histogram code recently. ::: ipc/chromium/src/base/histogram.h (Diff revisions 1 - 2) > }; > > -//------------------------------------------------------------------------------ > - > -// CustomHistogram is a histogram for a set of custom integers. > -class CustomHistogram : public Histogram { Nice catch, this is actually unused. ::: toolkit/components/telemetry/gen-histogram-data.py:156 (Diff revisions 1 - 2) > # For now we use this as a special cache only for exponential histograms, > # which require exp and log calls that show up in profiles. Initialization > # of other histograms also shows up in profiles, but it's unlikely that we > # would see much speedup since calculating their buckets is fairly trivial, > # and grabbing them from static data would likely incur a CPU cache miss. This comment needs a small update.
Attachment #8905656 -
Flags: review?(gfritzsche) → review+
Updated•7 years ago
|
Flags: needinfo?(gfritzsche)
Attachment #8905656 -
Flags: feedback?(chutten)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8905656 [details] Bug 1397376 - Avoid copying on initializing histogram ranges https://reviewboard.mozilla.org/r/177450/#review183458 I like it, seconding georg's point about the out-of-date comment.
Updated•7 years ago
|
Attachment #8905656 -
Flags: feedback?(chutten) → feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/34d4d0192028 Deduplicate histogram static bucket data r=gfritzsche https://hg.mozilla.org/integration/autoland/rev/ba149daae44e Reduce size of gExponentialBucketLowerBoundIndex r=gfritzsche https://hg.mozilla.org/integration/autoland/rev/447ae1ff1b04 Avoid copying on initializing histogram ranges r=gfritzsche
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34d4d0192028 https://hg.mozilla.org/mozilla-central/rev/ba149daae44e https://hg.mozilla.org/mozilla-central/rev/447ae1ff1b04
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•