Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2463223002:
Store field trial parameters in shared memory (Closed)
|
Created:
4 years, 1 month ago by lawrencewu Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore field trial parameters in shared memory
This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like:
TrialName
GroupName
ParamKey1
ParamValue1
ParamKey2
ParamValue2
...
BUG=660041
Committed: https://crrev.com/68995897ee15b012c33a3419d7740f1561643bc2
Cr-Commit-Position: refs/heads/master@{#435615}
Patch Set 1 #Patch Set 2 : refactor #Patch Set 3 : refactor #
Total comments: 10
Patch Set 4 : address comments and write test #
Total comments: 8
Patch Set 5 : address comments #
Total comments: 1
Patch Set 6 : get field trial parameters working #Patch Set 7 : add test and actually write params to pickle #Patch Set 8 : expand comments #Patch Set 9 : git rebase-update #Patch Set 10 : add clear params from shared memory #Patch Set 11 : fix tests #
Total comments: 22
Patch Set 12 : address comments #Patch Set 13 : remove unnecessary import #Patch Set 14 : add check in test for new_params #Patch Set 15 : fix compilation error #
Total comments: 14
Patch Set 16 : address comments #
Total comments: 24
Patch Set 17 : address rest of comments #Patch Set 18 : fix WriteStringPair #Patch Set 19 : fix failing tests due to lock #
Total comments: 14
Patch Set 20 : address comments #Patch Set 21 : address comments #
Total comments: 7
Patch Set 22 : add locks #
Total comments: 4
Patch Set 23 : git rebase-update #Patch Set 24 : check that cache has been cleared in test #
Messages
Total messages: 97 (64 generated)
Description was changed from ========== Use shared memory for field trial parameters BUG=660041 ========== to ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... This change doesn't add support for getting the parameters out, however. That will be addressed in a later CL. BUG=660041 ==========
lawrencewu@chromium.org changed reviewers: + asvitkine@chromium.org
initial implementation
https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:55: bool activated; Hmm, I thought I left a comment on your previous CL to add a comment above this member. Looking at that CL, yes I did here: "Nit: Remove "It contains whether or not it's activated." and instead put a comment above the member." https://codereview.chromium.org/2449783007#msg13 Can you do it in this CL? https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:58: uint32_t size; Add a blank line below this and a comment about the function. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:195: trial_state.group_name.as_string(), ¶ms)) Nit: {}'s https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:200: pickle->WriteString(StringPiece(param.second)); Can you also add the changes to actually read from these to FieldTrialParamAssociator? I'd rather have both parts be in the same CL. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial_param_associator.h:41: bool GetFieldTrialParamsWhileLocked(const std::string& trial_name, This is not correct. The lock in question is the one in this object. Since it's private, the caller can't lock it. Why not just use GetFieldTrialParams()?
Address comments and wrote a unit test for params. This change makes GetFieldTrialParams work on child processes by first checking if the params are in its cache, falling back to shared memory if necessary, and then updating the cache. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:55: bool activated; On 2016/11/01 14:57:16, Alexei Svitkine (slow) wrote: > Hmm, I thought I left a comment on your previous CL to add a comment above this > member. > > Looking at that CL, yes I did here: > > "Nit: Remove "It contains whether or not it's activated." and instead put a > comment above the member." > > https://codereview.chromium.org/2449783007#msg13 > > Can you do it in this CL? > Oops, I must have missed that. Done. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:58: uint32_t size; On 2016/11/01 14:57:16, Alexei Svitkine (slow) wrote: > Add a blank line below this and a comment about the function. Done. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:195: trial_state.group_name.as_string(), ¶ms)) On 2016/11/01 14:57:16, Alexei Svitkine (slow) wrote: > Nit: {}'s Done. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial.cc:200: pickle->WriteString(StringPiece(param.second)); On 2016/11/01 14:57:16, Alexei Svitkine (slow) wrote: > Can you also add the changes to actually read from these to > FieldTrialParamAssociator? > > I'd rather have both parts be in the same CL. Done. https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/40001/base/metrics/field_tria... base/metrics/field_trial_param_associator.h:41: bool GetFieldTrialParamsWhileLocked(const std::string& trial_name, On 2016/11/01 14:57:16, Alexei Svitkine (slow) wrote: > This is not correct. > > The lock in question is the one in this object. Since it's private, the caller > can't lock it. > > Why not just use GetFieldTrialParams()? Fixed by overloading GetFieldTrialParams() and calling that.
https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:89: return false; Can you refactor the code to re-use the GetTrialAndGroupName()? Or maybe have a helper function called ReadStringPair()? Then it can be re-used in all the three places. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:91: for (int i = 0; i < kFieldTrialParamLimit; ++i) { Instead of having an arbitrary limit, I suggest just iterating until ReadStringPiece() fails. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:586: if (field_trial->ref_ == SharedPersistentMemoryAllocator::kReferenceNull) Please add comments above the early returns that explain in what cases these happen and why it makes sense. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial_param_associator.h:43: bool GetFieldTrialParams(const std::string& trial_name, Overloads are discouraged by the style guide. Change the name - e.g. add a ByGroupName suffix. Also expand comment since it's not quite the same as above. One important thing to mention is this version of the API doesn't activate the trial. This is actually what we want for this use case - since serializing to child processes shouldn't activate the trial. But it's important to explicitly note this in the comment.
Address comments. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:89: return false; On 2016/11/01 19:13:27, Alexei Svitkine (slow) wrote: > Can you refactor the code to re-use the GetTrialAndGroupName()? > > Or maybe have a helper function called ReadStringPair()? Then it can be re-used > in all the three places. Done. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:91: for (int i = 0; i < kFieldTrialParamLimit; ++i) { On 2016/11/01 19:13:27, Alexei Svitkine (slow) wrote: > Instead of having an arbitrary limit, I suggest just iterating until > ReadStringPiece() fails. Done. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial.cc:586: if (field_trial->ref_ == SharedPersistentMemoryAllocator::kReferenceNull) On 2016/11/01 19:13:27, Alexei Svitkine (slow) wrote: > Please add comments above the early returns that explain in what cases these > happen and why it makes sense. Done. https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/60001/base/metrics/field_tria... base/metrics/field_trial_param_associator.h:43: bool GetFieldTrialParams(const std::string& trial_name, On 2016/11/01 19:13:27, Alexei Svitkine (slow) wrote: > Overloads are discouraged by the style guide. Change the name - e.g. add a > ByGroupName suffix. Also expand comment since it's not quite the same as above. > > One important thing to mention is this version of the API doesn't activate the > trial. This is actually what we want for this use case - since serializing to > child processes shouldn't activate the trial. But it's important to explicitly > note this in the comment. Expanded the comment and changed the function to GetFieldTrialParamsWithGroupName().
Description was changed from ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... This change doesn't add support for getting the parameters out, however. That will be addressed in a later CL. BUG=660041 ========== to ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... BUG=660041 ==========
https://codereview.chromium.org/2463223002/diff/80001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2463223002/diff/80001/base/metrics/field_tria... base/metrics/field_trial.h:433: FieldTrialParamAssociator::FieldTrialParams* params); I worry about this API being public. It may mislead people into thinking they could use it, when it's meant to be internal. For example, someone may expect this to work from the browser process. Can you come up with a design that keeps it private? One thought I had was if we make it private but have a function in FieldTrialParamAssociator() to register a callback to that function, which can be done e.g. during the creation of the first FieldTrialList. Now, that approach will need to have the SetCallback function public - but a) that function is less likely to be mistaken by users for something useful and b) we can enforce that it can only be used once. ... Now, the other approach could be to merge the two classes - FieldTrialList and FieldTrialParamAssociator - but that possibly seems like a bigger change? Well, at least splitting the files would be since you need to update all the includes. Another option is to not split the files and just merge it in. I guess I'm open to the idea, but I'd have to see the CL. However, maybe to not expand the scope of this CL too much, go with the callback approach first for now?
On 2016/11/02 14:56:46, Alexei Svitkine (very slow) wrote: > https://codereview.chromium.org/2463223002/diff/80001/base/metrics/field_trial.h > File base/metrics/field_trial.h (right): > > https://codereview.chromium.org/2463223002/diff/80001/base/metrics/field_tria... > base/metrics/field_trial.h:433: FieldTrialParamAssociator::FieldTrialParams* > params); > I worry about this API being public. It may mislead people into thinking they > could use it, when it's meant to be internal. > > For example, someone may expect this to work from the browser process. Can you > come up with a design that keeps it private? > > One thought I had was if we make it private but have a function in > FieldTrialParamAssociator() to register a callback to that function, which can > be done e.g. during the creation of the first FieldTrialList. > > Now, that approach will need to have the SetCallback function public - but a) > that function is less likely to be mistaken by users for something useful and b) > we can enforce that it can only be used once. > > ... Now, the other approach could be to merge the two classes - FieldTrialList > and FieldTrialParamAssociator - but that possibly seems like a bigger change? > Well, at least splitting the files would be since you need to update all the > includes. Another option is to not split the files and just merge it in. I guess > I'm open to the idea, but I'd have to see the CL. > > However, maybe to not expand the scope of this CL too much, go with the callback > approach first for now? Haven't forgotten about this. The code is just getting kind of messy, so I'm going to try to break it up into a couple, more discrete parts.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Update: I think we can avoid the callback method and just rename GetParams to GetParamsFromSharedMemory which is less likely to be mistaken.
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: 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.
https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:107: if (!ReadStringPair(iter, &tmp, &tmp)) You're passing iter by value - which means it gets copied and its position being advance by ReadStringPair() doesn't modify iter in this function. Which means you'll end up having the first param/value pair end up being the field trial name and group name. You can fix this by passing iter by pointer. But please also add test coverage for this - i.e. check the size of new_params map in your test to make sure it's as expected. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:228: bool PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) { Add a comment. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:246: } Can you put this function right underneath struct FieldTrialEntry? https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:599: // process, which should have all the parameters. So if we got here, then we The first part of the comment is not the only reason why field_trial_allocator_ could be not set up. For example, shared memory impl may not be enabled. Or it's being called too early. If you're trying to be accurate in the comment, suggest listing the cases (you can use a bullet list). https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:605: SharedPersistentMemoryAllocator::Reference ref = field_trial->ref_; Nit: I'm not sure the local var is needed here - can just use field_trial->ref_ directly. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:606: if (!ref) How about a comment about when this wouldn't be set? https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:621: // the existing item. This seems like a lot of work to do. I'm not sure it's necessary - my suspicion is that most cases that clear params for testing also destroy the FieldTrialList object. If so, then this wouldn't be necessary. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:47: // call to AddToAllocatorWhileLocked(), which might call So one consequence of this set up is that all field trial parameters must be registered before any calls to group()/group_name(). I think this is OK with the current code - but let's document this requirement - at least at the call site where this matters - which is in variations_seed_processor.cc where it calls group() and group_name() - add comments above those that say it's important that this happens after params have been registered for the given trial. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:51: return FieldTrialList::GetParamsFromSharedMemory(field_trial, params); Please add a comment explaining the flow for future readers. Also, how about making this use GetFieldTrialParamsWithoutFallback()? i.e. get the group name outside the lock and then call that function and if it returns false, fallback to shared mem. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1191: shm.get()->Map(64 << 10); // Hardcoded, equal to kFieldTrialAllocationSize. Nit: Remove this diff. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1219: EXPECT_EQ(new_params["key1"], "value1"); The expected value should be on the left.
The CQ bit was checked by lawrencewu@chromium.org to run a CQ dry run
Address comments. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:107: if (!ReadStringPair(iter, &tmp, &tmp)) On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > You're passing iter by value - which means it gets copied and its position being > advance by ReadStringPair() doesn't modify iter in this function. > > Which means you'll end up having the first param/value pair end up being the > field trial name and group name. > > You can fix this by passing iter by pointer. But please also add test coverage > for this - i.e. check the size of new_params map in your test to make sure it's > as expected. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:228: bool PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) { On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > Add a comment. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:246: } On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > Can you put this function right underneath struct FieldTrialEntry? Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:599: // process, which should have all the parameters. So if we got here, then we On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > The first part of the comment is not the only reason why field_trial_allocator_ > could be not set up. For example, shared memory impl may not be enabled. Or it's > being called too early. > > If you're trying to be accurate in the comment, suggest listing the cases (you > can use a bullet list). Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:605: SharedPersistentMemoryAllocator::Reference ref = field_trial->ref_; On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > Nit: I'm not sure the local var is needed here - can just use field_trial->ref_ > directly. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:606: if (!ref) On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > How about a comment about when this wouldn't be set? Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial.cc:621: // the existing item. On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > This seems like a lot of work to do. I'm not sure it's necessary - my suspicion > is that most cases that clear params for testing also destroy the FieldTrialList > object. If so, then this wouldn't be necessary. Although that's possible, and would make this unnecessary, it would still be incorrect to call the calling functions ClearAllParamsForTesting() or ClearAllVariationParams() if it cleared the whole FieldTrialList object and not just the params. IMO, we should keep this. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:47: // call to AddToAllocatorWhileLocked(), which might call On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > So one consequence of this set up is that all field trial parameters must be > registered before any calls to group()/group_name(). I think this is OK with the > current code - but let's document this requirement - at least at the call site > where this matters - which is in variations_seed_processor.cc where it calls > group() and group_name() - add comments above those that say it's important that > this happens after params have been registered for the given trial. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:51: return FieldTrialList::GetParamsFromSharedMemory(field_trial, params); On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > Please add a comment explaining the flow for future readers. > > Also, how about making this use GetFieldTrialParamsWithoutFallback()? > > i.e. get the group name outside the lock and then call that function and if it > returns false, fallback to shared mem. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1191: shm.get()->Map(64 << 10); // Hardcoded, equal to kFieldTrialAllocationSize. On 2016/11/22 17:46:38, Alexei Svitkine (slow) wrote: > Nit: Remove this diff. Done. https://codereview.chromium.org/2463223002/diff/200001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1219: EXPECT_EQ(new_params["key1"], "value1"); On 2016/11/22 17:46:37, Alexei Svitkine (slow) wrote: > The expected value should be on the left. Fixed.
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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.
asvitkine@chromium.org changed reviewers: + bcwhite@chromium.org
+bcwhite for review too https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:673: void FieldTrialList::ClearParamsFromSharedMemoryForTesting() { Use the lock for this. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:679: SharedPersistentMemoryAllocator* allocator = FieldTrialAllocator Change the other types below to your new typedefs too. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:692: prev_entry->GetTrialAndGroupName(&trial_name, &group_name); Check return value? https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:711: uint32_t oldType = allocator->GetType(prev_ref); old_type https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.h:439: // it in |params|. This is talking about implementation detail. Please change this comment to talk about what it does without implementation detail - since it's a public function. Also, move these new functions to the end of the list of public functions - they shouldn't be in the middle of commonly used ones that are expected to be used by many callers. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:47: params)) {} https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1221: EXPECT_EQ(static_cast<unsigned long>(2), new_params.size()); Instead of the cast, I think you can do 2U
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/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:673: void FieldTrialList::ClearParamsFromSharedMemoryForTesting() { On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > Use the lock for this. Done. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:679: SharedPersistentMemoryAllocator* allocator = On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > FieldTrialAllocator > > Change the other types below to your new typedefs too. Done. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:692: prev_entry->GetTrialAndGroupName(&trial_name, &group_name); On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > Check return value? Done by continuing if we failed to get the names. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.cc:711: uint32_t oldType = allocator->GetType(prev_ref); On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > old_type Fixed. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial.h:439: // it in |params|. On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > This is talking about implementation detail. > > Please change this comment to talk about what it does without implementation > detail - since it's a public function. > > Also, move these new functions to the end of the list of public functions - they > shouldn't be in the middle of commonly used ones that are expected to be used by > many callers. Done. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:47: params)) On 2016/11/23 17:23:39, Alexei Svitkine (slow) wrote: > {} Done. https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/280001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1221: EXPECT_EQ(static_cast<unsigned long>(2), new_params.size()); On 2016/11/23 17:23:40, Alexei Svitkine (slow) wrote: > Instead of the cast, I think you can do 2U Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:87: StringPiece* trial_name, The name of this method is generic. If you use string1_out and string2_out for parameter names then you can reuse this method. Might as well create a WriteStringPair and use that, too. Or do away with this completely. It doesn't make much sense to have one but not the other. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:117: if (!iter.ReadStringPiece(&key)) Can you just reuse ReadStringPair here? iter must have some kind of "has more" flag you can test, right? https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:144: pickle->WriteString(StringPiece(param.first)); You're testing the return value of WriteString above but not here. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:978: field_trial->ref_, kFieldTrialType); Verify: GetAllocSize() >= sizeof(entry) + entry->size Since you're checking all the read/write results, you're concerned that the data might be bad so you check this in code as well and not in a DCHECK. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:986: AutoLock auto_lock(global_->lock_); The comment in the .h file says lock_ protects access to registered_ but you're not accessing that in this method. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1016: new_entry->size = pickle.size(); Don't you need to copy the pickle data? https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1023: uint32_t old_type = allocator->GetType(prev_ref); You already know from line 995 that the type is kFieldTrialType. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:40: // We first try looking for the params in field_trial_params_. field_trial_params_ isn't accessed in the code so add comment "via FieldTrialList"... or whatever is actually correct. Or don't mention that field at all. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:41: // If that is unsuccessful, then we try to look it up in shared memory. Lines 43-44 say: if unsuccessful then return https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.h:45: // process, and even then you should probably just use GetFieldTrialParams(). So... When _should_ this method be used?
Address comments. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:87: StringPiece* trial_name, On 2016/11/23 21:03:52, bcwhite wrote: > The name of this method is generic. If you use string1_out and string2_out for > parameter names then you can reuse this method. > > Might as well create a WriteStringPair and use that, too. Or do away with this > completely. It doesn't make much sense to have one but not the other. I don't think we can totally reuse this method, since we'll miss out on the case where we can't read anymore, and we'll return false there. I'm not opposed to the WriteStringPair method though -- Changed to add that. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:117: if (!iter.ReadStringPiece(&key)) On 2016/11/23 21:03:52, bcwhite wrote: > Can you just reuse ReadStringPair here? iter must have some kind of "has more" > flag you can test, right? Unfortunately, no, there doesn't seem to be a "has more" method: https://cs.chromium.org/chromium/src/base/pickle.h?q=PickleIterator&sq=packag... https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:144: pickle->WriteString(StringPiece(param.first)); On 2016/11/23 21:03:52, bcwhite wrote: > You're testing the return value of WriteString above but not here. Fixed. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:978: field_trial->ref_, kFieldTrialType); On 2016/11/23 21:03:52, bcwhite wrote: > Verify: GetAllocSize() >= sizeof(entry) + entry->size > > Since you're checking all the read/write results, you're concerned that the data > might be bad so you check this in code as well and not in a DCHECK. Added. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:986: AutoLock auto_lock(global_->lock_); On 2016/11/23 21:03:52, bcwhite wrote: > The comment in the .h file says lock_ protects access to registered_ but you're > not accessing that in this method. Changed comment to reflect that lock_ also protects access to field_trial_allocator_. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1016: new_entry->size = pickle.size(); On 2016/11/23 21:03:52, bcwhite wrote: > Don't you need to copy the pickle data? Whoops, good catch -- yes, fixed. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1023: uint32_t old_type = allocator->GetType(prev_ref); On 2016/11/23 21:03:52, bcwhite wrote: > You already know from line 995 that the type is kFieldTrialType. Fixed. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:40: // We first try looking for the params in field_trial_params_. On 2016/11/23 21:03:52, bcwhite wrote: > field_trial_params_ isn't accessed in the code so add comment "via > FieldTrialList"... or whatever is actually correct. Or don't mention that field > at all. Changed to say "via GetFieldTrialParamsWithoutFallback", since it is actually accessing it from there. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:41: // If that is unsuccessful, then we try to look it up in shared memory. On 2016/11/23 21:03:52, bcwhite wrote: > Lines 43-44 say: if unsuccessful then return I cleared it up a bit to say: "If it's not there, then try to look it up in shared memory." and moved the comment down so it's more obvious what lines of code this comment is talking about. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.h (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial_param_associator.h:45: // process, and even then you should probably just use GetFieldTrialParams(). On 2016/11/23 21:03:52, bcwhite wrote: > So... When _should_ this method be used? Pretty much never, except for where it's currently used (in PickleFieldTrial()). But since that's in a different module this has to be public.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:154: StringPiece(param.second))) Nit: {}'s https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.h:231: friend class FieldTrialParamAssociator; Can this be removed? https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.h:556: // |params|. Add a second sentence mentioning that this is exposed only for it's used from FieldTrialParamAssociator and shouldn't be used by anything else. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:45: // GetFieldTrialParamsWithoutFallback. If it's not there, then try to look it Nit: The first sentence doesn't add much to the comment. How about merging the two sentences into something more concise, such as: // First try the local map, falling back to getting it from shared memory.
https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:117: if (!iter.ReadStringPiece(&key)) On 2016/11/23 21:49:20, lawrencewu wrote: > On 2016/11/23 21:03:52, bcwhite wrote: > > Can you just reuse ReadStringPair here? iter must have some kind of "has > more" > > flag you can test, right? > > Unfortunately, no, there doesn't seem to be a "has more" method: > https://cs.chromium.org/chromium/src/base/pickle.h?q=PickleIterator&sq=packag... You can reuse the existing method with a little creativity. if (!ReadStringPair(&key, &value)) return key.empty(); // Non-empty is bad: got one of a pair. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1016: new_entry->size = pickle.size(); On 2016/11/23 21:49:20, lawrencewu wrote: > On 2016/11/23 21:03:52, bcwhite wrote: > > Don't you need to copy the pickle data? > > Whoops, good catch -- yes, fixed. If your tests didn't catch that then your tests are incomplete. Make sure there is a test that stores and restores a complete set of data. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:153: if (!WriteStringPair(pickle, StringPiece(param.first), Do you have to manually create the StringPiece object? Most string things will auto-convert to that. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:989: CHECK_GE(global_->field_trial_allocator_->GetAllocSize(field_trial->ref_), You should just "return false" like every other failure since they all indicate the same thing: some sort of corruption has been detected. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:1188: NOTREACHED(); This will do nothing in production code meaning that it will continue to execute below. If that's a problem then you should also return.
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/2463223002/diff/300001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:117: if (!iter.ReadStringPiece(&key)) On 2016/11/24 18:29:47, bcwhite wrote: > On 2016/11/23 21:49:20, lawrencewu wrote: > > On 2016/11/23 21:03:52, bcwhite wrote: > > > Can you just reuse ReadStringPair here? iter must have some kind of "has > > more" > > > flag you can test, right? > > > > Unfortunately, no, there doesn't seem to be a "has more" method: > > > https://cs.chromium.org/chromium/src/base/pickle.h?q=PickleIterator&sq=packag... > > You can reuse the existing method with a little creativity. > > if (!ReadStringPair(&key, &value)) > return key.empty(); // Non-empty is bad: got one of a pair. That's very clever -- done. https://codereview.chromium.org/2463223002/diff/300001/base/metrics/field_tri... base/metrics/field_trial.cc:1016: new_entry->size = pickle.size(); On 2016/11/24 18:29:47, bcwhite wrote: > On 2016/11/23 21:49:20, lawrencewu wrote: > > On 2016/11/23 21:03:52, bcwhite wrote: > > > Don't you need to copy the pickle data? > > > > Whoops, good catch -- yes, fixed. > > If your tests didn't catch that then your tests are incomplete. Make sure there > is a test that stores and restores a complete set of data. Done. I found another bug while writing the test, too. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:153: if (!WriteStringPair(pickle, StringPiece(param.first), On 2016/11/24 18:29:47, bcwhite wrote: > Do you have to manually create the StringPiece object? Most string things will > auto-convert to that. Removed. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:154: StringPiece(param.second))) On 2016/11/24 17:56:32, Alexei Svitkine (slow) wrote: > Nit: {}'s Don't need anymore as removing the StringPiece ctors makes it not multi-line anymore. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:989: CHECK_GE(global_->field_trial_allocator_->GetAllocSize(field_trial->ref_), On 2016/11/24 18:29:47, bcwhite wrote: > You should just "return false" like every other failure since they all indicate > the same thing: some sort of corruption has been detected. Done. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.cc:1188: NOTREACHED(); On 2016/11/24 18:29:47, bcwhite wrote: > This will do nothing in production code meaning that it will continue to execute > below. If that's a problem then you should also return. Done. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial.h (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.h:231: friend class FieldTrialParamAssociator; On 2016/11/24 17:56:32, Alexei Svitkine (slow) wrote: > Can this be removed? Yes, I think so. Removed. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial.h:556: // |params|. On 2016/11/24 17:56:32, Alexei Svitkine (slow) wrote: > Add a second sentence mentioning that this is exposed only for it's used from > FieldTrialParamAssociator and shouldn't be used by anything else. Done. https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... File base/metrics/field_trial_param_associator.cc (right): https://codereview.chromium.org/2463223002/diff/360001/base/metrics/field_tri... base/metrics/field_trial_param_associator.cc:45: // GetFieldTrialParamsWithoutFallback. If it's not there, then try to look it On 2016/11/24 17:56:32, Alexei Svitkine (slow) wrote: > Nit: The first sentence doesn't add much to the comment. > > How about merging the two sentences into something more concise, such as: > > // First try the local map, falling back to getting it from shared memory. Done.
https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:976: if (!global_->field_trial_allocator_) field_trial_allocator_ shouldn't be used outside the lock. Can you use a lock around this function? https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:998: if (!global_ || !global_->field_trial_allocator_) field_trial_allocator_ shouldn't be checked outside the lock. Split the if.
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...
Add locks. https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:976: if (!global_->field_trial_allocator_) On 2016/11/24 21:58:48, Alexei Svitkine (slow) wrote: > field_trial_allocator_ shouldn't be used outside the lock. Can you use a lock > around this function? Done. https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:998: if (!global_ || !global_->field_trial_allocator_) On 2016/11/24 21:58:48, Alexei Svitkine (slow) wrote: > field_trial_allocator_ shouldn't be checked outside the lock. Split the if. Done.
lgtm, but please also wait for Brian's review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bcwhite: friendly ping On 2016/11/24 22:18:07, Alexei Svitkine (slow) wrote: > lgtm, but please also wait for Brian's review
https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:1008: // List of refs to eventually be made iterable. We can't make it in the loop, It shouldn't. You're iterating for kFieldTrialType but the last thing you do is reset the type to zero (0). Also, iteration only moves forward so you can change current/past without affecting what is returned in the past. But I think you can omit the MakeIterable completely. You don't care to be able to iterate over cleared entries... but even if you do, it was already made iterable on line #1222 after it was first allocated. Once done it's never undone. https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1304: EXPECT_EQ("*Trial1/Group1/", check_string); Don't you need to check that params come through here, too, and not just the field-trial name?
https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:1008: // List of refs to eventually be made iterable. We can't make it in the loop, On 2016/11/28 21:57:41, bcwhite wrote: > It shouldn't. You're iterating for kFieldTrialType but the last thing you do is > reset the type to zero (0). Also, iteration only moves forward so you can > change current/past without affecting what is returned in the past. > > But I think you can omit the MakeIterable completely. You don't care to be able > to iterate over cleared entries... but even if you do, it was already made > iterable on line #1222 after it was first allocated. Once done it's never > undone. I'm trying to make iterable the newly-allocated entries, not the cleared ones, though. If I don't make it iterable then we can't access it anymore, and if I do it in here, then we'll end up in a loop of allocating and then iterating over that new piece until we run out of space. https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1304: EXPECT_EQ("*Trial1/Group1/", check_string); On 2016/11/28 21:57:41, bcwhite wrote: > Don't you need to check that params come through here, too, and not just the > field-trial name? We cleared the params, so we're verifying that the trial/group name came through here but not the params (we checked that the params were removed on line 1288.
On 2016/11/28 22:51:27, lawrencewu wrote: > https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... > File base/metrics/field_trial.cc (right): > > https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... > base/metrics/field_trial.cc:1008: // List of refs to eventually be made > iterable. We can't make it in the loop, > On 2016/11/28 21:57:41, bcwhite wrote: > > It shouldn't. You're iterating for kFieldTrialType but the last thing you do > is > > reset the type to zero (0). Also, iteration only moves forward so you can > > change current/past without affecting what is returned in the past. > > > > But I think you can omit the MakeIterable completely. You don't care to be > able > > to iterate over cleared entries... but even if you do, it was already made > > iterable on line #1222 after it was first allocated. Once done it's never > > undone. > > I'm trying to make iterable the newly-allocated entries, not the cleared ones, > though. If I don't make it iterable then we can't access it anymore, and if I do > it in here, then we'll end up in a loop of allocating and then iterating over > that new piece until we run out of space. > > https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... > File base/metrics/field_trial_unittest.cc (right): > > https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... > base/metrics/field_trial_unittest.cc:1304: EXPECT_EQ("*Trial1/Group1/", > check_string); > On 2016/11/28 21:57:41, bcwhite wrote: > > Don't you need to check that params come through here, too, and not just the > > field-trial name? > > We cleared the params, so we're verifying that the trial/group name came through > here but not the params (we checked that the params were removed on line 1288. bcwhite@: mind giving an lgtm, if there are no remaining issues?
https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2463223002/diff/400001/base/metrics/field_tri... base/metrics/field_trial.cc:1008: // List of refs to eventually be made iterable. We can't make it in the loop, On 2016/11/28 22:51:26, lawrencewu wrote: > On 2016/11/28 21:57:41, bcwhite wrote: > > It shouldn't. You're iterating for kFieldTrialType but the last thing you do > is > > reset the type to zero (0). Also, iteration only moves forward so you can > > change current/past without affecting what is returned in the past. > > > > But I think you can omit the MakeIterable completely. You don't care to be > able > > to iterate over cleared entries... but even if you do, it was already made > > iterable on line #1222 after it was first allocated. Once done it's never > > undone. > > I'm trying to make iterable the newly-allocated entries, not the cleared ones, > though. If I don't make it iterable then we can't access it anymore, and if I do > it in here, then we'll end up in a loop of allocating and then iterating over > that new piece until we run out of space. Ah, of course. Good, then. https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1304: EXPECT_EQ("*Trial1/Group1/", check_string); On 2016/11/28 22:51:26, lawrencewu wrote: > On 2016/11/28 21:57:41, bcwhite wrote: > > Don't you need to check that params come through here, too, and not just the > > field-trial name? > > We cleared the params, so we're verifying that the trial/group name came through > here but not the params (we checked that the params were removed on line 1288. Where do you check that params come through after a complete clearing of the cache?
https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2463223002/diff/420001/base/metrics/field_tri... base/metrics/field_trial_unittest.cc:1304: EXPECT_EQ("*Trial1/Group1/", check_string); On 2016/11/30 15:05:33, bcwhite wrote: > On 2016/11/28 22:51:26, lawrencewu wrote: > > On 2016/11/28 21:57:41, bcwhite wrote: > > > Don't you need to check that params come through here, too, and not just the > > > field-trial name? > > > > We cleared the params, so we're verifying that the trial/group name came > through > > here but not the params (we checked that the params were removed on line 1288. > > Where do you check that params come through after a complete clearing of the > cache? That's in the test right above this one, AssociateFieldTrialParams. We create some params, clear the cache, and then fetch them from shared memory and check they came through.
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 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...
Brian's comment made me notice that we weren't checking that the cache was actually cleared in a test, so I added that check.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/30 18:03:50, Alexei Svitkine (slow) wrote: > lgtm bcwhite@: ptal
lgtm
The CQ bit was checked by lawrencewu@chromium.org
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": 460001, "attempt_start_ts": 1480606150375690,
"parent_rev": "68c1b5dd08e618f0e9694165b3cacb9b06590843", "commit_rev":
"4b109d9d9728236e1f6555c27ad4430eb0666d30"}
Message was sent while issue was closed.
Description was changed from ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... BUG=660041 ========== to ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... BUG=660041 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... BUG=660041 ========== to ========== Store field trial parameters in shared memory This CL writes field trial parameters into its allocated spot in shared memory. The format of the pickle should now look like: TrialName GroupName ParamKey1 ParamValue1 ParamKey2 ParamValue2 ... BUG=660041 Committed: https://crrev.com/68995897ee15b012c33a3419d7740f1561643bc2 Cr-Commit-Position: refs/heads/master@{#435615} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/68995897ee15b012c33a3419d7740f1561643bc2 Cr-Commit-Position: refs/heads/master@{#435615} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
