Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Created:
7 months, 2 weeks ago by bcwhite
Modified:
6 months, 3 weeks 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
7 months, 2 weeks 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 ...
7 months, 2 weeks 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) ...
7 months, 2 weeks 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, ...
7 months, 1 week 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 ...
7 months, 1 week 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: ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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
7 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
7 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. ...
7 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
6 months, 3 weeks ago (2017-05-01 16:38:01 UTC) #84
commit-bot: I haz the power
6 months, 3 weeks 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 efc10ee0f