Chromium Code Reviews
Help | Chromium Project | Sign in
(45)

Issue 2811713003: Embed a single sample in histogram metadata.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 4 days ago by bcwhite
Modified:
5 days, 3 hours 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-Commit-Position: refs/heads/master@{#466401} Committed: https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc2572337a039b79fdb9

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1044 lines, -229 lines) Patch
M base/metrics/histogram.h View 1 2 3 11 chunks +21 lines, -25 lines 0 comments Download
M base/metrics/histogram.cc View 1 2 3 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 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 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
M base/metrics/sample_vector_unittest.cc View 1 2 3 4 5 6 7 8 8 chunks +266 lines, -9 lines 0 comments Download
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 73 (58 generated)
bcwhite
2 weeks, 4 days 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 ...
2 weeks, 2 days 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) ...
2 weeks, 2 days 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, ...
1 week, 3 days 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 ...
1 week, 3 days 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: ...
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week, 1 day 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 ...
1 week, 1 day 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 ...
1 week, 1 day 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 ...
1 week, 1 day 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 ...
1 week, 1 day 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
1 week 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
1 week ago (2017-04-21 18:58:17 UTC) #71
tdanderson
1 week ago (2017-04-21 23:18:39 UTC) #72
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in
https://codereview.chromium.org/2832333002/ by tdanderson@chromium.org.

The reason for reverting is: Speculative revert for suspected cause of failing
NonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug test on
Mac10.9 Tests (dbg).

See
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%....
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46