Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(437)

Issue 2811713003: Embed a single sample in histogram metadata. (Closed)

Created:
3 years, 8 months ago by bcwhite
Modified:
3 years, 7 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Embed a single sample in histogram metadata. This uses the new DelayedPersistentAllocation to avoid allocating the full "counts" array when only one value is being stored. That one value is kept in the HistogramSamples metadata. When a second value is recorded, the counts-array is created and the single-sample is moved; it is there-after unused. This change only affects SampleVector. A future change may add the same support for PersistentSampleMap. A quick test shows persistent memory use is reduced by 44% (from 924 KiB to 512 KiB) after a clean start and the loading of three pages: chrome://histograms https://www.google.com/ chrome://histograms (again) BUG=705342 Review-Url: https://codereview.chromium.org/2811713003 Cr-Original-Commit-Position: refs/heads/master@{#466401} Committed: https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc2572337a039b79fdb9 Review-Url: https://codereview.chromium.org/2811713003 Cr-Commit-Position: refs/heads/master@{#468325} Committed: https://chromium.googlesource.com/chromium/src/+/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb

Patch Set 1 #

Patch Set 2 : rebased #

Patch Set 3 : fixed build problems #

Total comments: 14

Patch Set 4 : addressed review comments by asvitkine #

Total comments: 45

Patch Set 5 : addressed review comments by asvitkine #

Patch Set 6 : move single-sample from 'value' to 'bucket' #

Total comments: 24

Patch Set 7 : addressed review comments by asvitkine #

Total comments: 18

Patch Set 8 : addressed review comments by asvitkine #

Patch Set 9 : rebased #

Patch Set 10 : remove test temporarily to check mac bots (will submit as follow-on CL) #

Patch Set 11 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+778 lines, -220 lines) Patch
M base/metrics/histogram.h View 1 2 3 4 5 6 7 8 9 10 11 chunks +21 lines, -25 lines 0 comments Download
M base/metrics/histogram.cc View 1 2 3 4 5 6 7 8 9 10 10 chunks +65 lines, -59 lines 0 comments Download
M base/metrics/histogram_samples.h View 1 2 3 4 5 4 chunks +106 lines, -13 lines 0 comments Download
M base/metrics/histogram_samples.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +165 lines, -16 lines 0 comments Download
M base/metrics/histogram_unittest.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M base/metrics/persistent_histogram_allocator.cc View 1 2 3 4 5 6 7 8 5 chunks +30 lines, -23 lines 0 comments Download
M base/metrics/persistent_sample_map.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -2 lines 0 comments Download
M base/metrics/sample_map.cc View 1 chunk +1 line, -2 lines 0 comments Download
M base/metrics/sample_vector.h View 1 2 3 4 5 6 2 chunks +96 lines, -18 lines 0 comments Download
M base/metrics/sample_vector.cc View 1 2 3 4 5 6 7 2 chunks +290 lines, -59 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 87 (70 generated)
bcwhite
3 years, 8 months ago (2017-04-10 20:54:14 UTC) #8
Alexei Svitkine (slow)
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram.h File base/metrics/histogram.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram.h#newcode149 base/metrics/histogram.h:149: HistogramSamples::Metadata* logged_meta, Non-const params should be last - so ...
3 years, 8 months ago (2017-04-12 17:38:07 UTC) #12
bcwhite
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram.h File base/metrics/histogram.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram.h#newcode149 base/metrics/histogram.h:149: HistogramSamples::Metadata* logged_meta, On 2017/04/12 17:38:06, Alexei Svitkine (very slow) ...
3 years, 8 months ago (2017-04-13 00:06:01 UTC) #20
bcwhite
Using a simple local test of loading two pages: https://www.google.com/ chrome://histograms Without the single-sample optimization, ...
3 years, 8 months ago (2017-04-18 19:51:56 UTC) #23
Alexei Svitkine (slow)
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_samples.h File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_samples.h#newcode42 base/metrics/histogram_samples.h:42: int16_t count; On 2017/04/13 00:06:01, bcwhite wrote: > On ...
3 years, 8 months ago (2017-04-18 20:52:21 UTC) #24
bcwhite
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_samples.h File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_samples.h#newcode42 base/metrics/histogram_samples.h:42: int16_t count; On 2017/04/18 20:52:19, Alexei Svitkine (slow) wrote: ...
3 years, 8 months ago (2017-04-19 18:13:03 UTC) #30
bcwhite
https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_samples.h File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_samples.h#newcode41 base/metrics/histogram_samples.h:41: int16_t value; On 2017/04/19 18:13:02, bcwhite wrote: > On ...
3 years, 8 months ago (2017-04-19 20:10:23 UTC) #33
bcwhite
Switched from "value" to "bucket". This saved another 72KiB, too... though these measurements aren't all ...
3 years, 8 months ago (2017-04-20 19:07:26 UTC) #44
Alexei Svitkine (slow)
https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram.cc#newcode523 base/metrics/histogram.cc:523: new PersistentSampleVector(HashMetricName(name), ranges, meta, counts)); Wait, why is this ...
3 years, 8 months ago (2017-04-20 19:56:15 UTC) #45
bcwhite
https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram.cc#newcode523 base/metrics/histogram.cc:523: new PersistentSampleVector(HashMetricName(name), ranges, meta, counts)); On 2017/04/20 19:56:10, Alexei ...
3 years, 8 months ago (2017-04-20 21:18:08 UTC) #50
Alexei Svitkine (slow)
LGTM % comments I'm a little worried about what happens if there's a subtle bug ...
3 years, 8 months ago (2017-04-20 22:11:34 UTC) #51
bcwhite
>I'm a little worried about what happens if there's a subtle bug and some >histograms ...
3 years, 8 months ago (2017-04-21 13:25:32 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2811713003/240001
3 years, 8 months ago (2017-04-21 18:28:21 UTC) #68
commit-bot: I haz the power
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc2572337a039b79fdb9
3 years, 8 months ago (2017-04-21 18:58:17 UTC) #71
tdanderson
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/2832333002/ by tdanderson@chromium.org. ...
3 years, 8 months ago (2017-04-21 23:18:39 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2811713003/280001
3 years, 7 months ago (2017-05-01 16:38:01 UTC) #84
commit-bot: I haz the power
3 years, 7 months ago (2017-05-01 16:43:46 UTC) #87
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/fa8485bd1548ab76854d5d01ea2c...

Powered by Google App Engine
This is Rietveld 408576698