|
|
Created:
3 years, 8 months ago by bcwhite Modified:
3 years, 7 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEmbed 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 #
Dependent Patchsets: Messages
Total messages: 87 (70 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Embed a single-value 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. BUG=705342 ========== to ========== Embed a single-value 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 37% (from 924 KiB to 585 KiB) after a clean start and the loading of three pages: chrome://histograms, https://www.google.com/, and chrome://histograms again. BUG=705342 ==========
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.... base/metrics/histogram.h:149: HistogramSamples::Metadata* logged_meta, Non-const params should be last - so add the new params above the two meta's. Same below. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.cc:18: const int32_t kDisabledSingleSample = -1; These need comments. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.cc:251: : SingleSampleIterator(min, max, count, kSizeMax) {} Nit: Add an empty line between ctors and methods - here and after line 262 and so forth. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.h:42: int16_t count; Have you considered making them mutually exclusive? e.g. have a bit indicate whether it's one or the other. Then, have the remaining 31 bits hold the value - either of the single sample - or the count. The relevant bit could be just the sign bit to make checks simple and so it can just be an int32_t. (Though we should then be careful to never overflow max int32_t and flip the bit unintentionally - which would require extra overflow checks - which I think would be good to have regardless). https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... base/metrics/sample_vector_unittest.cc:24: const HistogramBase::AtomicCount* GetSamplesCounts( Nit: Seems like this can just be a free-standing function in the anon namespace in this file and then we can keep the tests as TEST()'s rather than TEST_F()'s.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
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.... base/metrics/histogram.h:149: HistogramSamples::Metadata* logged_meta, On 2017/04/12 17:38:06, Alexei Svitkine (very slow) wrote: > Non-const params should be last - so add the new params above the two meta's. > Same below. <sigh> There were arranged like that but I figured they grouped better this way. Done. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.cc:18: const int32_t kDisabledSingleSample = -1; On 2017/04/12 17:38:06, Alexei Svitkine (very slow) wrote: > These need comments. Done. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.cc:251: : SingleSampleIterator(min, max, count, kSizeMax) {} On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > Nit: Add an empty line between ctors and methods - here and after line 262 and > so forth. Done. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.h:42: int16_t count; On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > Have you considered making them mutually exclusive? > > e.g. have a bit indicate whether it's one or the other. Then, have the remaining > 31 bits hold the value - either of the single sample - or the count. > > The relevant bit could be just the sign bit to make checks simple and so it can > just be an int32_t. (Though we should then be careful to never overflow max > int32_t and flip the bit unintentionally - which would require extra overflow > checks - which I think would be good to have regardless). They can't be mutually exclusive because both are used together. You can't have a "count" without a "value". There has to be something to which the count applies. It could be split differently (like 8/24) using a bit-field but I thought this was cleaner. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... base/metrics/sample_vector_unittest.cc:24: const HistogramBase::AtomicCount* GetSamplesCounts( On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > Nit: Seems like this can just be a free-standing function in the anon namespace > in this file and then we can keep the tests as TEST()'s rather than TEST_F()'s. It's accessing a protected method off of SampleVectorBase and so needs to a friend of that class. Anonymous methods can't be friends. I find friend-ing the framework class to much cleaner than friend-ing every test that needs it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Using a simple local test of loading two pages: https://www.google.com/ chrome://histograms Without the single-sample optimization, persistent histogram usage was 872K. With a single 16/16 (value/count bits) sample, persistent usage was 464K. With a single 28/4 (value/count bits) sample, persistent usage was 555K. The amount of space used will certainly increase as new samples require allocation of the full "counts" array but reducing "count" space for increased "value" space seems to use more memory instead of less.
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.h:42: int16_t count; On 2017/04/13 00:06:01, bcwhite wrote: > On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > > Have you considered making them mutually exclusive? > > > > e.g. have a bit indicate whether it's one or the other. Then, have the > remaining > > 31 bits hold the value - either of the single sample - or the count. > > > > The relevant bit could be just the sign bit to make checks simple and so it > can > > just be an int32_t. (Though we should then be careful to never overflow max > > int32_t and flip the bit unintentionally - which would require extra overflow > > checks - which I think would be good to have regardless). > > They can't be mutually exclusive because both are used together. You can't have > a "count" without a "value". There has to be something to which the count > applies. > > It could be split differently (like 8/24) using a bit-field but I thought this > was cleaner. Per offline discussion, 16/16 seems to produce good results - so sticking with this SGTM. Can you expand the comment to explicitly mention both use cases? (e.g. a single value logged to a numeric histogram and and enum histogram being recorded multiple times with a single value - like a success) https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... base/metrics/sample_vector_unittest.cc:24: const HistogramBase::AtomicCount* GetSamplesCounts( On 2017/04/13 00:06:01, bcwhite wrote: > On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > > Nit: Seems like this can just be a free-standing function in the anon > namespace > > in this file and then we can keep the tests as TEST()'s rather than > TEST_F()'s. > > It's accessing a protected method off of SampleVectorBase and so needs to a > friend of that class. Anonymous methods can't be friends. I find friend-ing > the framework class to much cleaner than friend-ing every test that needs it. Ah, I missed that it was using a protected method. Mention that in a comment - i.e. that the class is used for friend access to protected methods. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:17: const size_t kSizeMax = std::numeric_limits<size_t>::max(); Nit: Can this be constexpr? https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:21: const int32_t kDisabledSingleSample = -1; Nit: constexpr https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:79: single_sample.as_atomic = 0; This seems like a strange thing to do. Wouldn't it be better for this to return false and take the single_sample as a ptr - so that the caller can check for failure - rather than sillently give them 0's? https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:112: subtle::Atomic32 existing, found; Nit: Separate lines. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:115: if (existing == kDisabledSingleSample) if Load() returned a bool and took a param, then this could use that rather than doing its own Acquire_load() https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:119: if (single_sample.as_parts.value != value16) Nit: Add a comment for semantic meaning of this check. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:122: if (count16 < 0) { Instead of your own checks, can you use base/numerics/safe_math.h? I think you can do CheckAdd() and check .IsValid() on the result. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:134: single_sample.as_parts.value = value16; Nit: Add a comment here mentioning no need to check for count overflow here since it was checked at the start of the function. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:151: // is called for the data members in that case. This adds a requirement that all members must be valid and make sense when all 0's. For example, this precludes expanding Metadata with objects that can't support this. Please add a comment about that - e.g. above Metadata struct, to clearly state this requirement. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:41: int16_t value; uint16_t for both? Or is there a valid case for them to be negative? https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:72: // but isn't available on all builds. Please expand these field comments to mention that it's a union - since that's easy to miss given it's way at the top. Also mention kDisabledSingleSample as the special value to mean "disabled". https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:114: // Because sturctures held in persistent memory must be POD, there can be no Nit: structures is misspelled https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:209: // SampleCountIterator implementation: Nit: Just // SampleCounterIterator: (For new code). https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:47: if (!counts()) { Nit: The multi-sample case is less code - so reverse the if so that the early return case is simpler and this one can be unindented. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:63: MountCountsStorage(); Can the name of this function be changed so it's clear it's also doing the MoveSingleSample() as part of it? Also, I'm not keen on Mount name - as it's not commonly used and can be confusing - how about "Allocate"? https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:153: // Don't call AccumulateSingleSample because that udpates sum and count updates https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:177: while (index < counts_size_) { Nested loops make this hard to follow. I suggest refactoring the inner loop into a helper function. Actually, it seems the inner loop is just finding the bucket index. Can it just use GetBucketIndex() instead? It would also give us log(n) performance instead of linear due to binary search. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:192: if (index == counts_size_) Add a comment about this case. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:235: // a invalid (including zero) "value" will crash. Nit: an invalid https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:246: // There are many of SampleVector objects and the lock is needed very Nit: remove "of" https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:297: if (persistent_counts_.reference()) Nit: {} Should this just call MountExistingCountsStorage() since this is the same logic? https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... File base/metrics/sample_vector.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.h:54: void MoveSingleSampleToCounts(); Please add short comments for this and the three functions below, since this is complicated code.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/histogram_... base/metrics/histogram_samples.h:42: int16_t count; On 2017/04/18 20:52:19, Alexei Svitkine (slow) wrote: > On 2017/04/13 00:06:01, bcwhite wrote: > > On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > > > Have you considered making them mutually exclusive? > > > > > > e.g. have a bit indicate whether it's one or the other. Then, have the > > remaining > > > 31 bits hold the value - either of the single sample - or the count. > > > > > > The relevant bit could be just the sign bit to make checks simple and so it > > can > > > just be an int32_t. (Though we should then be careful to never overflow max > > > int32_t and flip the bit unintentionally - which would require extra > overflow > > > checks - which I think would be good to have regardless). > > > > They can't be mutually exclusive because both are used together. You can't > have > > a "count" without a "value". There has to be something to which the count > > applies. > > > > It could be split differently (like 8/24) using a bit-field but I thought this > > was cleaner. > > Per offline discussion, 16/16 seems to produce good results - so sticking with > this SGTM. Can you expand the comment to explicitly mention both use cases? > (e.g. a single value logged to a numeric histogram and and enum histogram being > recorded multiple times with a single value - like a success) Done. https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/40001/base/metrics/sample_vec... base/metrics/sample_vector_unittest.cc:24: const HistogramBase::AtomicCount* GetSamplesCounts( On 2017/04/18 20:52:19, Alexei Svitkine (slow) wrote: > On 2017/04/13 00:06:01, bcwhite wrote: > > On 2017/04/12 17:38:07, Alexei Svitkine (very slow) wrote: > > > Nit: Seems like this can just be a free-standing function in the anon > > namespace > > > in this file and then we can keep the tests as TEST()'s rather than > > TEST_F()'s. > > > > It's accessing a protected method off of SampleVectorBase and so needs to a > > friend of that class. Anonymous methods can't be friends. I find friend-ing > > the framework class to much cleaner than friend-ing every test that needs it. > > Ah, I missed that it was using a protected method. Mention that in a comment - > i.e. that the class is used for friend access to protected methods. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:17: const size_t kSizeMax = std::numeric_limits<size_t>::max(); On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: Can this be constexpr? Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:21: const int32_t kDisabledSingleSample = -1; On 2017/04/18 20:52:19, Alexei Svitkine (slow) wrote: > Nit: constexpr Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:79: single_sample.as_atomic = 0; On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > This seems like a strange thing to do. Wouldn't it be better for this to return > false and take the single_sample as a ptr - so that the caller can check for > failure - rather than sillently give them 0's? When a value is "extracted", it becomes zero. When it's extracted/disabled, it's still zero to the outside even though a different "flag" value is held inside. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:112: subtle::Atomic32 existing, found; On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: Separate lines. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:115: if (existing == kDisabledSingleSample) On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > if Load() returned a bool and took a param, then this could use that rather than > doing its own Acquire_load() Load() returns the parts structure. This needs the atomic value for later comparison. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:119: if (single_sample.as_parts.value != value16) On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: Add a comment for semantic meaning of this check. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:122: if (count16 < 0) { On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Instead of your own checks, can you use base/numerics/safe_math.h? > > I think you can do CheckAdd() and check .IsValid() on the result. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:134: single_sample.as_parts.value = value16; On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: Add a comment here mentioning no need to check for count overflow here > since it was checked at the start of the function. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.cc:151: // is called for the data members in that case. On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > This adds a requirement that all members must be valid and make sense when all > 0's. For example, this precludes expanding Metadata with objects that can't > support this. > > Please add a comment about that - e.g. above Metadata struct, to clearly state > this requirement. Done (in the .h file for "struct Metadata"). https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:41: int16_t value; On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > uint16_t for both? > > Or is there a valid case for them to be negative? It was significantly simpler this way because the full-size values are signed. Defining these as unsigned made for signed/unsigned additions and comparisons. I can take another look at it now that the rest of the code is stable if you think it's worth it. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:72: // but isn't available on all builds. On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Please expand these field comments to mention that it's a union - since that's > easy to miss given it's way at the top. > > Also mention kDisabledSingleSample as the special value to mean "disabled". Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:114: // Because sturctures held in persistent memory must be POD, there can be no On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: structures is misspelled Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:209: // SampleCountIterator implementation: On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: Just // SampleCounterIterator: > > (For new code). Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:47: if (!counts()) { On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: The multi-sample case is less code - so reverse the if so that the early > return case is simpler and this one can be unindented. This case falls through to the multi-sample case if accumulation cannot be done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:63: MountCountsStorage(); On 2017/04/18 20:52:21, Alexei Svitkine (slow) wrote: > Can the name of this function be changed so it's clear it's also doing the > MoveSingleSample() as part of it? Sure. > Also, I'm not keen on Mount name - as it's not commonly used and can be > confusing - how about "Allocate"? Except that it has (conceptually at least and maybe for real) already been allocated. I don't mind, really, but "mount" (like you would a filesystem) is a pretty accurate description. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:153: // Don't call AccumulateSingleSample because that udpates sum and count On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > updates Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:177: while (index < counts_size_) { On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nested loops make this hard to follow. > > I suggest refactoring the inner loop into a helper function. Could do though there is state (index) that is preserved across calls which will make it a little less clean. > Actually, it seems > the inner loop is just finding the bucket index. Can it just use > GetBucketIndex() instead? It would also give us log(n) performance instead of > linear due to binary search. Using: N == number of items it the source (iterator) M == number of ranges in this vector (counts) GetBucketIndex would be N-log-M whereas this is just M (actually max(N,M) but M>N) since both the iterator and index are scanning forward. Which is more efficient would depend on the relative sizes of N and M. The current method is also tolerant if the counts in the iterator aren't valid for |this| but GetBucketIndex would CHECK. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:192: if (index == counts_size_) On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Add a comment about this case. Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:235: // a invalid (including zero) "value" will crash. On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: an invalid Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:246: // There are many of SampleVector objects and the lock is needed very On 2017/04/18 20:52:21, Alexei Svitkine (slow) wrote: > Nit: remove "of" Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.cc:297: if (persistent_counts_.reference()) On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > Nit: {} > > Should this just call MountExistingCountsStorage() since this is the same logic? Done. https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... File base/metrics/sample_vector.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/sample_vec... base/metrics/sample_vector.h:54: void MoveSingleSampleToCounts(); On 2017/04/18 20:52:21, Alexei Svitkine (slow) wrote: > Please add short comments for this and the three functions below, since this is > complicated code. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... File base/metrics/histogram_samples.h (right): https://codereview.chromium.org/2811713003/diff/80001/base/metrics/histogram_... base/metrics/histogram_samples.h:41: int16_t value; On 2017/04/19 18:13:02, bcwhite wrote: > On 2017/04/18 20:52:20, Alexei Svitkine (slow) wrote: > > uint16_t for both? > > > > Or is there a valid case for them to be negative? > > It was significantly simpler this way because the full-size values are signed. > Defining these as unsigned made for signed/unsigned additions and comparisons. > > I can take another look at it now that the rest of the code is stable if you > think it's worth it. Another option is to change it from "value" to "bucket" which would add (actually restore) determining the bucket index for single samples. I may have to do this anyway to deal with various add/subtract operations which use iterators which in turn operate on bucket ranges rather than actual values. Thinking on it...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #6 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Embed a single-value 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 37% (from 924 KiB to 585 KiB) after a clean start and the loading of three pages: chrome://histograms, https://www.google.com/, and chrome://histograms again. BUG=705342 ========== to ========== 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/, and chrome://histograms again. BUG=705342 ==========
Description was changed from ========== 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/, and chrome://histograms again. BUG=705342 ========== to ========== 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 ==========
Switched from "value" to "bucket". This saved another 72KiB, too... though these measurements aren't all that rigorous or accurate.
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... base/metrics/histogram.cc:523: new PersistentSampleVector(HashMetricName(name), ranges, meta, counts)); Wait, why is this switching to PersistentSampleVector? https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:119: subtle::Atomic32 found; Nit: How about renaming found to updated_value - so it's clearer what it's referring to (when you use it later). https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:131: // The single sample was zero so becomes the |bucket| parameter, the Nit: "single sample" -> "|single_sample|" to make it clear it's referring to the old value - and not the new single sample. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:132: // contents of which where checked above to fit in 16 bits. Nit: where -> were https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:57: if (counts()) Maybe we should rename counts() to GetCounts() - as otherwise it looks like a simple accessor (rather than one that uses atomic ops) and thus could be mistaken for this functions' code not being correct since it doesn't look like it's loading things using acquire_load. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:153: if (iter->GetBucketIndex(&iter_index)) What happens if this returns false? I don't see the comment discussing that explicitly and why it's correct to have index_offset being 0 in that case. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:154: index_offset = dest_index - iter_index; index_offset isn't used until the while() loop at the end of function - please move this to right above that. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:165: // Don't call AccumulateSingleSample because that updates sum and count How about adding a param to AccumulateSingleSample() to control updating sum/count? This way, more code could be re-used. Also, I think the race condition check below if counts() got allocated in the mean time should be moved into that function too - so that it's not duplicated in multiple places. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.h (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.h:104: const size_t counts_size_; Do we need this as a separate field or can we get rid of this given we can get this from the bucket_ranges_ object? (I was going to originally suggest to add a comment about this always being initialized even when counts isn't... but seems simpler to just get rid of it and just use bucket_ranges_?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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... base/metrics/histogram.cc:523: new PersistentSampleVector(HashMetricName(name), ranges, meta, counts)); On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > Wait, why is this switching to PersistentSampleVector? Previously, the SampleVector had two ctors, one that told it to use internal storage and one that provided outside storage. I've broken it into two separate classes with a common base class. This means that the (local) SampleVector doesn't need to store an DelayedPersistentAllocation object and the PersistentSampleVector doesn't need the local_counts_ vector, even if these are empty. More importantly, virtual functions provide the correct ways of managing these storage mechanisms. The Histogram class, however, still has separate ctors for either local or persistent storage; this particular one is the persistent version. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:119: subtle::Atomic32 found; On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > Nit: How about renaming found to updated_value - so it's clearer what it's > referring to (when you use it later). I think "updated_value" could be construed as the value _after_ the CompareAndSwap when in fact it is always the value found during the compare but before the swap. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:131: // The single sample was zero so becomes the |bucket| parameter, the On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > Nit: "single sample" -> "|single_sample|" to make it clear it's referring to the > old value - and not the new single sample. Done. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:132: // contents of which where checked above to fit in 16 bits. On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > Nit: where -> were Done. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:57: if (counts()) On 2017/04/20 19:56:13, Alexei Svitkine (slow) wrote: > Maybe we should rename counts() to GetCounts() - as otherwise it looks like a > simple accessor (rather than one that uses atomic ops) and thus could be > mistaken for this functions' code not being correct since it doesn't look like > it's loading things using acquire_load. I still see it as a simple accessor; it's just retrieving the value with no modifications or side effects. I don't think it being an atomic access makes a difference. I think GetCounts() would make it look like a heavier method and, by the style guide, have to be make an out-of-line method which could create additional overhead. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:153: if (iter->GetBucketIndex(&iter_index)) On 2017/04/20 19:56:12, Alexei Svitkine (slow) wrote: > What happens if this returns false? I don't see the comment discussing that > explicitly and why it's correct to have index_offset being 0 in that case. Done. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:154: index_offset = dest_index - iter_index; On 2017/04/20 19:56:11, Alexei Svitkine (slow) wrote: > index_offset isn't used until the while() loop at the end of function - please > move this to right above that. GetBucketIndex() has to be called before Next() and Next() has to be called before the single-value storage case that checks Done(). I could save the result from GetBucketIndex into a bool and do the condition based on that later on, but I think that reduces readability and not worth the cost of delaying the simple calculation of index_offset. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:165: // Don't call AccumulateSingleSample because that updates sum and count On 2017/04/20 19:56:14, Alexei Svitkine (slow) wrote: > How about adding a param to AccumulateSingleSample() to control updating > sum/count? This way, more code could be re-used. I think that would be confusing as there is no "value" here to pass. It would amount to making up a value (i.e. "min") but telling the method not to use it. Calling it would also add a slight additional overhead of another method call and remove nothing of the code surrounding it here. > Also, I think the race condition check below if counts() got allocated in the > mean time should be moved into that function too - so that it's not duplicated > in multiple places. HistogramSamples, which implements AccumulateSingleSample, has no knowledge of "counts". That is a SampleVector concept. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.h (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.h:104: const size_t counts_size_; On 2017/04/20 19:56:14, Alexei Svitkine (slow) wrote: > Do we need this as a separate field or can we get rid of this given we can get > this from the bucket_ranges_ object? > > (I was going to originally suggest to add a comment about this always being > initialized even when counts isn't... but seems simpler to just get rid of it > and just use bucket_ranges_?) Done.
LGTM % comments I'm a little worried about what happens if there's a subtle bug and some histograms start being off. Wondering if it's at all possible to run this as an experiment first - although the changes seem pretty intrusive - e.g. to AddSubtractImpl() - so maybe not easy to put behind an experiment. Do we have good test coverage of AddSubtractImpl()? 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... base/metrics/histogram.cc:523: new PersistentSampleVector(HashMetricName(name), ranges, meta, counts)); On 2017/04/20 21:18:07, bcwhite wrote: > On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > > Wait, why is this switching to PersistentSampleVector? > > Previously, the SampleVector had two ctors, one that told it to use internal > storage and one that provided outside storage. > > I've broken it into two separate classes with a common base class. > > This means that the (local) SampleVector doesn't need to store an > DelayedPersistentAllocation object and the PersistentSampleVector doesn't need > the local_counts_ vector, even if these are empty. > > More importantly, virtual functions provide the correct ways of managing these > storage mechanisms. > > The Histogram class, however, still has separate ctors for either local or > persistent storage; this particular one is the persistent version. Makes sense - thanks for the explanation! https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:119: subtle::Atomic32 found; On 2017/04/20 21:18:07, bcwhite wrote: > On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > > Nit: How about renaming found to updated_value - so it's clearer what it's > > referring to (when you use it later). > > I think "updated_value" could be construed as the value _after_ the > CompareAndSwap when in fact it is always the value found during the compare but > before the swap. True, updated_value isn't a great name. How about to make the code clear - you can declare |found| where you call CompareAndSwap() and add a "bool value_updated" at the top that you set to "value_updated = (found != existing);" and loop on while (!value_updated). This makes the code a bit easier to follow. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:154: index_offset = dest_index - iter_index; On 2017/04/20 21:18:08, bcwhite wrote: > On 2017/04/20 19:56:11, Alexei Svitkine (slow) wrote: > > index_offset isn't used until the while() loop at the end of function - please > > move this to right above that. > > GetBucketIndex() has to be called before Next() and Next() has to be called > before the single-value storage case that checks Done(). > > I could save the result from GetBucketIndex into a bool and do the condition > based on that later on, but I think that reduces readability and not worth the > cost of delaying the simple calculation of index_offset. Ah! Thanks for expanding the comment to mention this, much clearer now. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:165: // Don't call AccumulateSingleSample because that updates sum and count On 2017/04/20 21:18:07, bcwhite wrote: > On 2017/04/20 19:56:14, Alexei Svitkine (slow) wrote: > > How about adding a param to AccumulateSingleSample() to control updating > > sum/count? This way, more code could be re-used. > > I think that would be confusing as there is no "value" here to pass. It would > amount to making up a value (i.e. "min") but telling the method not to use it. > > Calling it would also add a slight additional overhead of another method call > and remove nothing of the code surrounding it here. > > > > Also, I think the race condition check below if counts() got allocated in the > > mean time should be moved into that function too - so that it's not duplicated > > in multiple places. > > HistogramSamples, which implements AccumulateSingleSample, has no knowledge of > "counts". That is a SampleVector concept. I see. I still see this duplication of logic between the two places as unfortunate. Maybe we can think about a better way to structure this in a following CL. I guess the single value is in HistogramSamples so that in the future we could potentially use it for SampleMap as well? Even though benefits there aren't presumably as much... As otherwise this CL is doing it just for sample vector. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:81: for (size_t i = counts_size(); i > 0; --i, ++counts_array) { Nit: Did you switch the loop structure just so we only call counts_size() once? If so, I suggest keeping the old structure and just assigning it to a local variable. This way, it's still a simple normal for loop and the reader isn't wondering if there's a reason we're iterating backwards here (and we don't need to increment both the ptr and i). https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:130: HistogramBase::Count count; Nit: Move these closer to where they're used - ie. above the first Get() call https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:132: size_t iter_index; Nit: This one and index_offset can be declared right above the iter->GetBucketIndex() call. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:141: dest_index = GetBucketIndex(min); Nit: This can be declared on this line rather than above. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:150: // a given iterator object, index_offset is either set here and used below, Nit: |index_offset| https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:154: index_offset = dest_index - iter_index; So this logic only works if it's the same number of buckets. Can we add some checking about that? I know you check min/max later in the loop... but maybe we should DCHECK() earlier on bucket counts? https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:312: // with another block) while another instance continues to update to the I don't understand the "because it shares a memory segment with another block" logic. Do you mean when this object is created over an existing mmaped file where the data was created by another run? If so, I would make that more explicit in the comment. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:323: // There is no check that counts is not yet mounted because, given that this Nit: Make it a DCHECK just in case. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector_unittest.cc:296: TEST_F(SampleVectorTest, SingleSampleTest) { Nit: I'd omit the word "Test" from all of these. I think it's already implied they're tests. So just SingleSample, PersistentSampleVectorWithOutsideAlloc, etc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
>I'm a little worried about what happens if there's a subtle bug and some >histograms start being off. Wondering if it's at all possible to run this as an >experiment first - although the changes seem pretty intrusive - e.g. to >AddSubtractImpl() - so maybe not easy to put behind an experiment. Do we have >good test coverage of AddSubtractImpl()? It's pretty good and I've added coverage for the single-sample cases. Also, all tests that merge histograms from files and subprocesses or flatten things from the StatisticsRecorder indirectly test AddSubtractImpl(). This CL depends on the DelayedPersistentAllocation review: https://codereview.chromium.org/2806403002/ https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/histogram... base/metrics/histogram_samples.cc:119: subtle::Atomic32 found; On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > On 2017/04/20 21:18:07, bcwhite wrote: > > On 2017/04/20 19:56:10, Alexei Svitkine (slow) wrote: > > > Nit: How about renaming found to updated_value - so it's clearer what it's > > > referring to (when you use it later). > > > > I think "updated_value" could be construed as the value _after_ the > > CompareAndSwap when in fact it is always the value found during the compare > but > > before the swap. > > True, updated_value isn't a great name. How about to make the code clear - you > can declare |found| where you call CompareAndSwap() and add a "bool > value_updated" at the top that you set to "value_updated = (found != existing);" > and loop on while (!value_updated). This makes the code a bit easier to follow. Done. I've also changed the names from existing/found to original/existing with a comment of how it works. https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/180001/base/metrics/sample_ve... base/metrics/sample_vector.cc:165: // Don't call AccumulateSingleSample because that updates sum and count > > > Also, I think the race condition check below if counts() got allocated in > the > > > mean time should be moved into that function too - so that it's not > duplicated > > > in multiple places. > > > > HistogramSamples, which implements AccumulateSingleSample, has no knowledge of > > "counts". That is a SampleVector concept. > > I guess the single value is in HistogramSamples so that in the future we could > potentially use it for SampleMap as well? Even though benefits there aren't > presumably as much... That's part of it (since it's a generic concept) but also because HistogramSamples is what manages the metadata (sum, redundant-count, etc.). https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... File base/metrics/sample_vector.cc (right): https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:81: for (size_t i = counts_size(); i > 0; --i, ++counts_array) { On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: Did you switch the loop structure just so we only call counts_size() once? > If so, I suggest keeping the old structure and just assigning it to a local > variable. > > This way, it's still a simple normal for loop and the reader isn't wondering if > there's a reason we're iterating backwards here (and we don't need to increment > both the ptr and i). Done. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:130: HistogramBase::Count count; On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: Move these closer to where they're used - ie. above the first Get() call Done. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:132: size_t iter_index; On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: This one and index_offset can be declared right above the > iter->GetBucketIndex() call. Done. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:141: dest_index = GetBucketIndex(min); On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: This can be declared on this line rather than above. Done. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:150: // a given iterator object, index_offset is either set here and used below, On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: |index_offset| Done. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:154: index_offset = dest_index - iter_index; On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > So this logic only works if it's the same number of buckets. Can we add some > checking about that? > > I know you check min/max later in the loop... but maybe we should DCHECK() > earlier on bucket counts? It'll work with different numbers of buckets so long as the destination is a superset of the source. There's an existing test just for that. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:312: // with another block) while another instance continues to update to the On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > I don't understand the "because it shares a memory segment with another block" > logic. > > Do you mean when this object is created over an existing mmaped file where the > data was created by another run? If so, I would make that more explicit in the > comment. Something else. Comment updated. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector.cc:323: // There is no check that counts is not yet mounted because, given that this On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: Make it a DCHECK just in case. It's allowed to be called when it is already mounted; race conditions could cause it. There's just no early-exit for that case. Comment updated. https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... File base/metrics/sample_vector_unittest.cc (right): https://codereview.chromium.org/2811713003/diff/200001/base/metrics/sample_ve... base/metrics/sample_vector_unittest.cc:296: TEST_F(SampleVectorTest, SingleSampleTest) { On 2017/04/20 22:11:34, Alexei Svitkine (slow) wrote: > Nit: I'd omit the word "Test" from all of these. I think it's already implied > they're tests. So just SingleSample, PersistentSampleVectorWithOutsideAlloc, > etc. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2811713003/#ps240001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492799277130930, "parent_rev": "7a0ea2253643c495276d4f5953871318f706509a", "commit_rev": "4162bafec82ea11c98d4dc2572337a039b79fdb9"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/4162bafec82ea11c98d4dc257233... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc257233...
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%....
Message was sent while issue was closed.
Description was changed from ========== 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/+/4162bafec82ea11c98d4dc257233... ========== to ========== 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/+/4162bafec82ea11c98d4dc257233... ==========
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2811713003/#ps280001 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493656641948050, "parent_rev": "2f53aa1c96d14295e7739cae01db7d780f3dcb44", "commit_rev": "fa8485bd1548ab76854d5d01ea2c4cd31d719aeb"}
Message was sent while issue was closed.
Description was changed from ========== 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/+/4162bafec82ea11c98d4dc257233... ========== to ========== 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/+/4162bafec82ea11c98d4dc257233... Review-Url: https://codereview.chromium.org/2811713003 Cr-Commit-Position: refs/heads/master@{#468325} Committed: https://chromium.googlesource.com/chromium/src/+/fa8485bd1548ab76854d5d01ea2c... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as https://chromium.googlesource.com/chromium/src/+/fa8485bd1548ab76854d5d01ea2c... |