Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490513004:
Bump field trial allocator size to 64kb and report usage (Closed)
|
Created:
4 years, 1 month ago by lawrencewu Modified:
4 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBump field trial allocator size to 64kb and report usage
This CL changes the size of the shared memory used by the field trial allocator
to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now
correctly reports the used size of the allocator in a histogram.
BUG=663415
Committed: https://crrev.com/29e9989bdde19978fc9e7b92b1c762f99d8b2cd5
Cr-Commit-Position: refs/heads/master@{#431071}
Patch Set 1 #Patch Set 2 : Bump field trial allocator size and report usage #
Total comments: 3
Patch Set 3 : Create new histogram and add comment #Patch Set 4 : move ifdef #Patch Set 5 : Add ifdef for unused variable #
Total comments: 16
Patch Set 6 : address comments #Patch Set 7 : remove unnecessary include #
Total comments: 2
Patch Set 8 : add comment for activated field #
Total comments: 2
Patch Set 9 : do !field_trial->ref_ #Patch Set 10 : whoops, should be field_trial->ref_ #Messages
Total messages: 47 (31 generated)
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
initial CL
Description was changed from ========== Bump field trial allocator size to 64kb and report usage BUG=663415 ========== to ========== Bump field trial allocator size to 64kb and report usage This CL changes the size of the shared memory used by the field trial allocator to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now correctly reports the used size of the allocator in a histogram. BUG=663415 ==========
https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (left): https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:731: UMA_HISTOGRAM_COUNTS_10000("UMA.FieldTrialAllocator.Size", Changing the macro without changing histogram name is not OK. It will renumber all the bucket ranges and now you'll have different buckets reported by different versions. So please rename the histogram, marking the old name as <obsolete> in the config. Can you also add a different histogram logging the percent used? https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:46: const size_t kFieldTrialAllocationSize = 4 << 14; // 64 KiB should be enough The "should be enough" comment is not useful. I suggest moving the comment to above the constant and expanding on it. In particular, mentioning that this doesn't want memory since parts of the block that are not touched don't get mapped. If you want to keep the "should be enough" comment, you should justify it - e.g. by referencing the observed amounts currently.
https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... File base/metrics/field_trial.cc (left): https://codereview.chromium.org/2490513004/diff/20001/base/metrics/field_tria... base/metrics/field_trial.cc:731: UMA_HISTOGRAM_COUNTS_10000("UMA.FieldTrialAllocator.Size", On 2016/11/08 18:15:23, Alexei Svitkine (very slow) wrote: > Changing the macro without changing histogram name is not OK. It will renumber > all the bucket ranges and now you'll have different buckets reported by > different versions. So please rename the histogram, marking the old name as > <obsolete> in the config. > > Can you also add a different histogram logging the percent used? For the rename, you can use "2" suffix to the name.
lawrencewu@chromium.org changed reviewers: + bcwhite@chromium.org
The CQ bit was checked by lawrencewu@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...
Changed the histogram name to "Used" and created "UsedPct". Also expanded on comment for kFieldTrialAllocatorSize.
lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by lawrencewu@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_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 lawrencewu@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/2490513004/#ps80001 (title: "Add ifdef for unused variable")
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
https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:51: const size_t kFieldTrialAllocationSize = 4 << 14; Please do this as 64 << 10 // 64 KiB https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:187: void ReportFieldTrialAllocatorUsage( Don't need this. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:744: ReportFieldTrialAllocatorUsage(global_->field_trial_allocator_.get()); When you create the allocator, call allocator->CreateTrackingHistograms(allocator->Name()); Here, call allocator->UpdateTrackingHistograms(); See here for how the histograms are named: https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator...
https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:45: const uint32_t kFieldTrialType = 0xABA17E13 + 1; // SHA1(FieldTrialEntry) v1 Blank line after. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:46: // We allocate 64 KiB to hold all the field trial data. This should be enough, Include the date when this was true. Also, comment what will happen if this isn't large enough. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:56: // object that we unpickle and read from. Comment that any changes to this structure require a bump in the TypeId version number defined at the top of this file. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:58: bool activated; This should be a fixed size (i.e. uint32_t) so that this structure is guaranteed to be identical across both 32/64 bit architectures. Otherwise, data written by one arch will not read correctly if read by a different one. "bool" is not strictly defined in its size.
The CQ bit was checked by lawrencewu@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 lawrencewu@chromium.org
The CQ bit was checked by lawrencewu@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...
Address comments. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:45: const uint32_t kFieldTrialType = 0xABA17E13 + 1; // SHA1(FieldTrialEntry) v1 On 2016/11/08 20:30:02, bcwhite wrote: > Blank line after. Done. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:46: // We allocate 64 KiB to hold all the field trial data. This should be enough, On 2016/11/08 20:30:02, bcwhite wrote: > Include the date when this was true. Also, comment what will happen if this > isn't large enough. Done. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:51: const size_t kFieldTrialAllocationSize = 4 << 14; On 2016/11/08 20:14:32, bcwhite wrote: > Please do this as > > 64 << 10 // 64 KiB Done. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:56: // object that we unpickle and read from. On 2016/11/08 20:30:02, bcwhite wrote: > Comment that any changes to this structure require a bump in the TypeId version > number defined at the top of this file. Done. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:58: bool activated; On 2016/11/08 20:30:02, bcwhite wrote: > This should be a fixed size (i.e. uint32_t) so that this structure is guaranteed > to be identical across both 32/64 bit architectures. Otherwise, data written by > one arch will not read correctly if read by a different one. > > "bool" is not strictly defined in its size. Done. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:187: void ReportFieldTrialAllocatorUsage( On 2016/11/08 20:14:32, bcwhite wrote: > Don't need this. Removed. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:744: ReportFieldTrialAllocatorUsage(global_->field_trial_allocator_.get()); On 2016/11/08 20:14:32, bcwhite wrote: > When you create the allocator, call > > allocator->CreateTrackingHistograms(allocator->Name()); > > Here, call > > allocator->UpdateTrackingHistograms(); > > > See here for how the histograms are named: > https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... Done. But this will get called on every subprocess launch. Is that okay?
lgtm https://codereview.chromium.org/2490513004/diff/120001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/120001/base/metrics/field_tri... base/metrics/field_trial.cc:63: uint32_t activated; Please add a comment above this field. Especially to explain that it's a boolean.
add comment https://codereview.chromium.org/2490513004/diff/120001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/120001/base/metrics/field_tri... base/metrics/field_trial.cc:63: uint32_t activated; On 2016/11/09 18:14:27, Alexei Svitkine (very slow) wrote: > Please add a comment above this field. Especially to explain that it's a > boolean. Somehow, this never actually gets done -- Done.
lgtm https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:744: ReportFieldTrialAllocatorUsage(global_->field_trial_allocator_.get()); On 2016/11/09 18:06:46, lawrencewu wrote: > On 2016/11/08 20:14:32, bcwhite wrote: > > When you create the allocator, call > > > > allocator->CreateTrackingHistograms(allocator->Name()); > > > > Here, call > > > > allocator->UpdateTrackingHistograms(); > > > > > > See here for how the histograms are named: > > > https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... > > Done. But this will get called on every subprocess launch. Is that okay? Not ideal but acceptable. If you can call it once after everything has been loaded, that would be best but it's not overly important since we're really just looking for the maximum value it reaches. https://codereview.chromium.org/2490513004/diff/140001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/140001/base/metrics/field_tri... base/metrics/field_trial.cc:917: if (field_trial->ref_ != SharedPersistentMemoryAllocator::kReferenceNull) It's acceptable to use just "!value" to check validity of things that could be "null" instead of an explicit comparison.
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Address comments. https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:744: ReportFieldTrialAllocatorUsage(global_->field_trial_allocator_.get()); On 2016/11/09 18:57:37, bcwhite wrote: > On 2016/11/09 18:06:46, lawrencewu wrote: > > On 2016/11/08 20:14:32, bcwhite wrote: > > > When you create the allocator, call > > > > > > allocator->CreateTrackingHistograms(allocator->Name()); > > > > > > Here, call > > > > > > allocator->UpdateTrackingHistograms(); > > > > > > > > > See here for how the histograms are named: > > > > > > https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator... > > > > Done. But this will get called on every subprocess launch. Is that okay? > > Not ideal but acceptable. If you can call it once after everything has been > loaded, that would be best but it's not overly important since we're really just > looking for the maximum value it reaches. Acknowledged. https://codereview.chromium.org/2490513004/diff/140001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2490513004/diff/140001/base/metrics/field_tri... base/metrics/field_trial.cc:917: if (field_trial->ref_ != SharedPersistentMemoryAllocator::kReferenceNull) On 2016/11/09 18:57:37, bcwhite wrote: > It's acceptable to use just "!value" to check validity of things that could be > "null" instead of an explicit comparison. Done.
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-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 lawrencewu@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 lawrencewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, bcwhite@chromium.org Link to the patchset: https://codereview.chromium.org/2490513004/#ps180001 (title: "whoops, should be field_trial->ref_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Bump field trial allocator size to 64kb and report usage This CL changes the size of the shared memory used by the field trial allocator to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now correctly reports the used size of the allocator in a histogram. BUG=663415 ========== to ========== Bump field trial allocator size to 64kb and report usage This CL changes the size of the shared memory used by the field trial allocator to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now correctly reports the used size of the allocator in a histogram. BUG=663415 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Bump field trial allocator size to 64kb and report usage This CL changes the size of the shared memory used by the field trial allocator to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now correctly reports the used size of the allocator in a histogram. BUG=663415 ========== to ========== Bump field trial allocator size to 64kb and report usage This CL changes the size of the shared memory used by the field trial allocator to 64kb from 4kb, since 4kb was too small and was causing crashes. It also now correctly reports the used size of the allocator in a histogram. BUG=663415 Committed: https://crrev.com/29e9989bdde19978fc9e7b92b1c762f99d8b2cd5 Cr-Commit-Position: refs/heads/master@{#431071} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/29e9989bdde19978fc9e7b92b1c762f99d8b2cd5 Cr-Commit-Position: refs/heads/master@{#431071} |
