Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449143002:
Actually update FieldTrialEntry's activated field (Closed)
|
Created:
4 years, 1 month ago by lawrencewu Modified:
4 years, 1 month ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionActually update FieldTrialEntry's activated field
This changes moves the creation of the field trial entry in shared
memory a earlier, in FinalizeGroupChoice(), and sets the "activated"
field on the entry in NotifyFieldTrialGroupSelection(). We still create
the allocator lazily.
BUG=659212
Committed: https://crrev.com/986a6385750dae1120a89c49be4ddf3f6a707ce0
Cr-Commit-Position: refs/heads/master@{#427749}
Patch Set 1 #Patch Set 2 : fix compile issue, clean up code #Patch Set 3 : add to the allocator if it's null #
Total comments: 6
Patch Set 4 : address comments #
Total comments: 4
Patch Set 5 : fix locking problem #
Total comments: 4
Patch Set 6 : fix nits #
Total comments: 2
Patch Set 7 : ActivateFieldTrialEntry += WhileLocked #Messages
Total messages: 28 (14 generated)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 lawrencewu@chromium.org to run a CQ dry run
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
Initial implementation for this feature.
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/2449143002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:348: FieldTrialList::GetFieldTrialAllocator(); I'm not a fan of having a public getter for the allocator - since that means anyone can get it and mess with it. So, to avoid this, let's turn add a static function on FieldTrialList for it. Note: It can be a wrapper around AddToAllocatorWhileLocked() since that function requires the caller to hold the lock (which I think you currently don't and so need to fix anyway) and then it can just access the allocator. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:797: SharedPersistentMemoryAllocator* allocator = Nit: Please make a helper function for this block of code. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:326: SharedPersistentMemoryAllocator::Reference ref_; Nit: Move this above the static var above - so all the non-static members are together.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address comments. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:348: FieldTrialList::GetFieldTrialAllocator(); On 2016/10/25 19:18:53, Alexei Svitkine (slow) wrote: > I'm not a fan of having a public getter for the allocator - since that means > anyone can get it and mess with it. > > So, to avoid this, let's turn add a static function on FieldTrialList for it. > > Note: It can be a wrapper around AddToAllocatorWhileLocked() since that function > requires the caller to hold the lock (which I think you currently don't and so > need to fix anyway) and then it can just access the allocator. I moved the AddToAllocatorWhileLocked function to be a static function on FieldTrialList, and it now holds its own lock. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:797: SharedPersistentMemoryAllocator* allocator = On 2016/10/25 19:18:53, Alexei Svitkine (slow) wrote: > Nit: Please make a helper function for this block of code. Done. https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.h:326: SharedPersistentMemoryAllocator::Reference ref_; On 2016/10/25 19:18:53, Alexei Svitkine (slow) wrote: > Nit: Move this above the static var above - so all the non-static members are > together. Done.
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); I don't think these should be in the public API, since we only want to use them internally. So I'd like to see a setup that doesn't expose them. So, FieldTrialList is a friend of FieldTrial - so FieldTrialList can use private function in FieldTrial. So one thing that could be done is having these functions still be members of FieldTrial and take any necessary parameters that could be passed in or return values. For example, AddToAllocator can take the pointer to the lock and to the allocator - or it can omit the pointer to the lock and require the caller to hold the lock - which might be reasonable. ActivateFieldTrialEntry() can take the allocator as well.
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/25 21:12:29, Alexei Svitkine (slow) wrote: > I don't think these should be in the public API, since we only want to use them > internally. > > So I'd like to see a setup that doesn't expose them. > > So, FieldTrialList is a friend of FieldTrial - so FieldTrialList can use private > function in FieldTrial. > > So one thing that could be done is having these functions still be members of > FieldTrial and take any necessary parameters that could be passed in or return > values. > > For example, AddToAllocator can take the pointer to the lock and to the > allocator - or it can omit the pointer to the lock and require the caller to > hold the lock - which might be reasonable. > > ActivateFieldTrialEntry() can take the allocator as well. Hmm, I'm not sure that solves the problem of adding the field trial entry from FieldTrial::FinalizeGroupChoice, because since it's a method in FieldTrial it has no global lock to pass in. We could possibly solve it by making FieldTrial a friend of FieldTrialList, and then making these two functions private. What do you think?
https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/26 13:43:15, lawrencewu wrote: > On 2016/10/25 21:12:29, Alexei Svitkine (slow) wrote: > > I don't think these should be in the public API, since we only want to use > them > > internally. > > > > So I'd like to see a setup that doesn't expose them. > > > > So, FieldTrialList is a friend of FieldTrial - so FieldTrialList can use > private > > function in FieldTrial. > > > > So one thing that could be done is having these functions still be members of > > FieldTrial and take any necessary parameters that could be passed in or return > > values. > > > > For example, AddToAllocator can take the pointer to the lock and to the > > allocator - or it can omit the pointer to the lock and require the caller to > > hold the lock - which might be reasonable. > > > > ActivateFieldTrialEntry() can take the allocator as well. > > Hmm, I'm not sure that solves the problem of adding the field trial entry from > FieldTrial::FinalizeGroupChoice, because since it's a method in FieldTrial it > has no global lock to pass in. > > We could possibly solve it by making FieldTrial a friend of FieldTrialList, and > then making these two functions private. What do you think? Ah right. Well, ok how about just having: OnGroupFinalized(FieldTrial*) in the FieldTrialList that FinalizeGroupChoice() will call. We still have a public static function, but it's sufficiently narrow scoped and you can add a comment that it's only meant to be by FieldTrial. Also the name doesn't suggest to callers that it's something useful for them to use (e.g. unlike ActivateFieldTrialEntry which might make people thing it does something the want).
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/2449143002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2449143002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.h:530: static void AddToAllocator(FieldTrial* field_trial); On 2016/10/26 14:49:02, Alexei Svitkine (slow) wrote: > On 2016/10/26 13:43:15, lawrencewu wrote: > > On 2016/10/25 21:12:29, Alexei Svitkine (slow) wrote: > > > I don't think these should be in the public API, since we only want to use > > them > > > internally. > > > > > > So I'd like to see a setup that doesn't expose them. > > > > > > So, FieldTrialList is a friend of FieldTrial - so FieldTrialList can use > > private > > > function in FieldTrial. > > > > > > So one thing that could be done is having these functions still be members > of > > > FieldTrial and take any necessary parameters that could be passed in or > return > > > values. > > > > > > For example, AddToAllocator can take the pointer to the lock and to the > > > allocator - or it can omit the pointer to the lock and require the caller to > > > hold the lock - which might be reasonable. > > > > > > ActivateFieldTrialEntry() can take the allocator as well. > > > > Hmm, I'm not sure that solves the problem of adding the field trial entry from > > FieldTrial::FinalizeGroupChoice, because since it's a method in FieldTrial it > > has no global lock to pass in. > > > > We could possibly solve it by making FieldTrial a friend of FieldTrialList, > and > > then making these two functions private. What do you think? > > Ah right. Well, ok how about just having: > > OnGroupFinalized(FieldTrial*) in the FieldTrialList that FinalizeGroupChoice() > will call. We still have a public static function, but it's sufficiently narrow > scoped and you can add a comment that it's only meant to be by FieldTrial. > > Also the name doesn't suggest to callers that it's something useful for them to > use (e.g. unlike ActivateFieldTrialEntry which might make people thing it does > something the want). Sounds good, done.
https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:811: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { Nit: Please order the functions the same way as in the .h file. https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:834: { Nit: No need for this block since the auto_lock applies to everything below it already.
Address nits. https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:811: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { On 2016/10/26 16:21:08, Alexei Svitkine (slow) wrote: > Nit: Please order the functions the same way as in the .h file. Done. https://codereview.chromium.org/2449143002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.cc:834: { On 2016/10/26 16:21:08, Alexei Svitkine (slow) wrote: > Nit: No need for this block since the auto_lock applies to everything below it > already. Fixed.
lgtm https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:903: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { Nit: ActivateFieldTrialEntryWhileLocked()
fix nit https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2449143002/diff/100001/base/metrics/field_tri... base/metrics/field_trial.cc:903: void FieldTrialList::ActivateFieldTrialEntry(FieldTrial* field_trial) { On 2016/10/26 16:37:20, Alexei Svitkine (slow) wrote: > Nit: ActivateFieldTrialEntryWhileLocked() Done.
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/2449143002/#ps120001 (title: "ActivateFieldTrialEntry += WhileLocked")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Actually update FieldTrialEntry's activated field This changes moves the creation of the field trial entry in shared memory a earlier, in FinalizeGroupChoice(), and sets the "activated" field on the entry in NotifyFieldTrialGroupSelection(). We still create the allocator lazily. BUG=659212 ========== to ========== Actually update FieldTrialEntry's activated field This changes moves the creation of the field trial entry in shared memory a earlier, in FinalizeGroupChoice(), and sets the "activated" field on the entry in NotifyFieldTrialGroupSelection(). We still create the allocator lazily. BUG=659212 Committed: https://crrev.com/986a6385750dae1120a89c49be4ddf3f6a707ce0 Cr-Commit-Position: refs/heads/master@{#427749} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/986a6385750dae1120a89c49be4ddf3f6a707ce0 Cr-Commit-Position: refs/heads/master@{#427749} |
