Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794073002:
De-duplicate ranges information in persistent memory. (Closed)
|
Created:
3 years, 8 months ago by bcwhite Modified:
3 years, 8 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. |
DescriptionDe-duplicate ranges information in persistent memory.
By having BucketRanges keep a reference to where the data can be found
in persistent memory, the existing de-dup of the BucketRanges object
also serves to avoid duplicating the data in persistent memory.
Simple testing shows allocated persistent histogram memory in the
browser is reduced by about 22%. This should be similar in all
processes though somewhat less as the number of histograms held goes
down.
BUG=705342
Review-Url: https://codereview.chromium.org/2794073002
Cr-Commit-Position: refs/heads/master@{#462897}
Committed: https://chromium.googlesource.com/chromium/src/+/af1913582fdf411b2f7edc84189c78b1fb402a37
Patch Set 1 #Patch Set 2 : restored missing erase in ForgetHistogramForTesting #Patch Set 3 : comment improvements #
Total comments: 7
Patch Set 4 : addressed review comments by asvitkine #
Total comments: 2
Patch Set 5 : addressed final review comments #
Messages
Total messages: 38 (27 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: 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 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...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sweet! Will review in a bit. Do you have any data about how much memory savings this provides? (e.g. you could just do some measurements locally)
Description was changed from ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. BUG=705342 ========== to ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG=705342 ==========
On 2017/04/04 15:35:08, Alexei Svitkine (slow) wrote: > Sweet! Will review in a bit. > > Do you have any data about how much memory savings this provides? (e.g. you > could just do some measurements locally) About 22%. Description updated.
How much is that in KB? On Tue, Apr 4, 2017 at 12:37 PM, <bcwhite@chromium.org> wrote: > On 2017/04/04 15:35:08, Alexei Svitkine (slow) wrote: > > Sweet! Will review in a bit. > > > > Do you have any data about how much memory savings this provides? (e.g. > you > > could just do some measurements locally) > > About 22%. Description updated. > > https://codereview.chromium.org/2794073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> How much is that in KB? About 261 KiB out of 1138 KiB used in the browser process after a fresh startup and loading three pages: chrome://histograms, https://www.google.com/, and again chrome://histograms. Normal usage is about 3x that; savings should scale linearly.
Description was changed from ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG=705342 ========== to ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG=705342 ==========
https://codereview.chromium.org/2794073002/diff/40001/base/metrics/bucket_ran... File base/metrics/bucket_ranges.h (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/bucket_ran... base/metrics/bucket_ranges.h:90: mutable subtle::Atomic32 persistent_reference_ = 0; I don't think this should be mutable. Can you just change the relevant functions to not receive the param by const ptr - but by regular ptr? https://codereview.chromium.org/2794073002/diff/40001/base/metrics/persistent... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/persistent... base/metrics/persistent_histogram_allocator.cc:361: HistogramBase::Sample* ranges_data = Should we also check that this is not null? https://codereview.chromium.org/2794073002/diff/40001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/statistics... base/metrics/statistics_recorder.cc:442: if (base->GetHistogramType() != SPARSE_HISTOGRAM) { Nit: Add a comment about this logic.
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/2794073002/diff/40001/base/metrics/bucket_ran... File base/metrics/bucket_ranges.h (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/bucket_ran... base/metrics/bucket_ranges.h:90: mutable subtle::Atomic32 persistent_reference_ = 0; On 2017/04/04 17:45:09, Alexei Svitkine (slow) wrote: > I don't think this should be mutable. Can you just change the relevant functions > to not receive the param by const ptr - but by regular ptr? The "const" goes all the way back to StatisticsRecorder's RegisterOrDeleteDuplicateRanges method and I don't think that should be changed. I think this is okay because though a reference is changing, the contents never do. Either the persistent contents don't exist or they match that of the const ranges. Even if the reference changes, it would always point to the same values, just different copies of it. In any case, the value can always be used with complete safety after reading just as though it was immutable. https://codereview.chromium.org/2794073002/diff/40001/base/metrics/persistent... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/persistent... base/metrics/persistent_histogram_allocator.cc:361: HistogramBase::Sample* ranges_data = On 2017/04/04 17:45:09, Alexei Svitkine (slow) wrote: > Should we also check that this is not null? Done. https://codereview.chromium.org/2794073002/diff/40001/base/metrics/statistics... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/statistics... base/metrics/statistics_recorder.cc:442: if (base->GetHistogramType() != SPARSE_HISTOGRAM) { On 2017/04/04 17:45:09, Alexei Svitkine (slow) wrote: > Nit: Add a comment about this logic. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM It occurred to me, however, that we're still duplicating the ranges data - as we keep it in both BucketRanges and persistent histograms shared memory. And that this is in fact a memory use regression with persistent histograms compared to before. I think (as a follow-up), this is something we should fix - i.e. having a BucketRanges object only have the persistent ref and not local storage. https://codereview.chromium.org/2794073002/diff/40001/base/metrics/bucket_ran... File base/metrics/bucket_ranges.h (right): https://codereview.chromium.org/2794073002/diff/40001/base/metrics/bucket_ran... base/metrics/bucket_ranges.h:90: mutable subtle::Atomic32 persistent_reference_ = 0; On 2017/04/05 13:41:30, bcwhite wrote: > On 2017/04/04 17:45:09, Alexei Svitkine (slow) wrote: > > I don't think this should be mutable. Can you just change the relevant > functions > > to not receive the param by const ptr - but by regular ptr? > > The "const" goes all the way back to StatisticsRecorder's > RegisterOrDeleteDuplicateRanges method and I don't think that should be changed. > > I think this is okay because though a reference is changing, the contents never > do. Either the persistent contents don't exist or they match that of the const > ranges. Even if the reference changes, it would always point to the same > values, just different copies of it. In any case, the value can always be used > with complete safety after reading just as though it was immutable. Fair enough. I guess we still want to keep the other members constant, but the way the API is designed they're not initialized at ctor time. Ideally, I think it would be great if the BucketRanges object could be made to have ranges_ and checksum_ fields const and initialized on construction and then we can make the pointer to it non-const with this field being changeable. However, that's a bigger change so I won't hold up this CL on it. https://codereview.chromium.org/2794073002/diff/60001/base/metrics/persistent... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2794073002/diff/60001/base/metrics/persistent... base/metrics/persistent_histogram_allocator.cc:371: ranges_ref = PersistentMemoryAllocator::kReferenceNull; Nit: = 0 Since the rest of your code uses 0 / checking in an if.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Good idea merging the in-ram and persistent bucket info. I'll do that in a follow-up CL. Maybe I can fix the const-ness at the same time. https://codereview.chromium.org/2794073002/diff/60001/base/metrics/persistent... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2794073002/diff/60001/base/metrics/persistent... base/metrics/persistent_histogram_allocator.cc:371: ranges_ref = PersistentMemoryAllocator::kReferenceNull; On 2017/04/05 20:57:51, Alexei Svitkine (slow) wrote: > Nit: = 0 > > Since the rest of your code uses 0 / checking in an if. I think it should be the other. References are generally treated as opaque values so "if (ref)" or "if (!ref)" is the norm. I've probably cheated in a few places and used zero; I'll have to keep a lookout for that.
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/2794073002/#ps80001 (title: "addressed final review comments")
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": 80001, "attempt_start_ts": 1491580311314260,
"parent_rev": "09fb058d6bfb0e74343a851f56e4032c44b56c19", "commit_rev":
"af1913582fdf411b2f7edc84189c78b1fb402a37"}
Message was sent while issue was closed.
Description was changed from ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG=705342 ========== to ========== De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG=705342 Review-Url: https://codereview.chromium.org/2794073002 Cr-Commit-Position: refs/heads/master@{#462897} Committed: https://chromium.googlesource.com/chromium/src/+/af1913582fdf411b2f7edc84189c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/af1913582fdf411b2f7edc84189c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
