Added a SetForced method to allow forcing a new set of group bucketting.

Added a SetForced method to allow forcing a new set of group bucketting.

This will be needed by the Finch client (soon to be known as the Variations Service) that will get updated information about FieldTrials and pre-feed the field trial list based on this information.

So this is different from CreateFieldTrial, which forced a group choice, this new method allows forcing a new set of appended groups.

BUG=121695
TEST=build\Debug\base_unittests.exe --gtest_filter=FieldTrialTest.SetForced


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135748

[MAD, 2012-05-04 20:02:15]

We need this to complete the Finch Client which will fetch updated information
about Field Trials from the server and use it to force a new registration of
Field Trials.

Initially, I thought we would use CreateFieldTrial for this, which create the
trials as forced. But we now want to let the Field Trial make the coin toss
still, so the Finch client create a factory trial, append the groups and then
mark the trial as forced so that the hard coded registration won't panic...

OK?

Thanks!

BYE
MAD...

[jar (doing other things), 2012-05-05 00:32:25]

https://chromiumcodereview.appspot.com/10382018/diff/2001/base/metrics/field_...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/2001/base/metrics/field_...
base/metrics/field_trial.cc:176: // change the bucketting into groups, so there
must have been at least one
I'm not clear what "change the buketting into groups" means.

It is also pretty unclear to me what et means to set forced_ after the fact,
given that we've presumably returned (possibly) something else during the
AppendGroup processing.

[MAD, 2012-05-05 12:25:03]

Sorry for not being clear, here's the complete picture.

Someone creates a Field Trial with 3 groups, default with 80% allocation, group
A with 10% and group B with 10%.

After the trial has run for a while, we decide to change the group allocation so
the server will return a new set of bucket sizes, e.g., default will now be 50%
and A & B will be 25%.

Once the client has read those values, it saves them in local state for the next
session. When that next session starts, we read the new bucket sizes from local
state, and create the Field Trial with the new values for the group allocation.
Then, we must identify that the trial was forced so that the hard coded
registration of the trial doesn't fail and the proper group ids are returned.

Makes sense?

Would this wording the comments work better?

Explicit forcing should only be for cases where we want to set the group
probabilities before the hard coded field trial setup is executed. So there must
have been at least one non-default group appended at that point.

[MAD, 2012-05-07 13:48:09]

Better like this?

https://chromiumcodereview.appspot.com/10382018/diff/2001/base/metrics/field_...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/2001/base/metrics/field_...
base/metrics/field_trial.cc:176: // change the bucketting into groups, so there
must have been at least one
On 2012/05/05 00:32:26, jar wrote:
> I'm not clear what "change the buketting into groups" means.
> 
> It is also pretty unclear to me what et means to set forced_ after the fact,
> given that we've presumably returned (possibly) something else during the
> AppendGroup processing.

Done.

[MAD, 2012-05-07 18:43:47]

Made a small modification, please take another look...

Thanks!

BYE
MAD...

[jar (doing other things), 2012-05-07 18:58:00]

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:176: if (forced_)
How does this handle "races?"  I'm curious about several folks trying ot do
forced on different threads (there is no apparent restriciton... and you mention
that other folks could have come sooner?).

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:184: group();
Here again, how does this play in an async case?  For instance, could be call
group() while someone else is still buildin the version up, and has not appended
all teh groups?

I'm very unclear about the (intended) semantics.

[MAD, 2012-05-07 19:11:24]

Answers to questions, let me know if it's still not clear...

Will try to come up with better comment language to better describe this.

Thanks!

BYE
MAD...

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:176: if (forced_)
On 2012/05/07 18:58:01, jar wrote:
> How does this handle "races?"  I'm curious about several folks trying ot do
> forced on different threads (there is no apparent restriciton... and you
mention
> that other folks could have come sooner?).
> 

From what I understand of this code, this is as racy as the rest of the
FieldTrial class which doesn't protect at all against setting and/or usage from
different threads.

Other folks could have created this FieldTrial sooner in the same thread... I
could add DCHECKs in all appropriate FieldTrial methods if you think we need
that...

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:184: group();
On 2012/05/07 18:58:01, jar wrote:
> Here again, how does this play in an async case?  For instance, could be call
> group() while someone else is still buildin the version up, and has not
appended
> all teh groups?
> 
> I'm very unclear about the (intended) semantics.

I'm not sure which "async case" you are talking about. This is all intended to
happen from the UI thread at initialization time. The semantics have not
changed, one must play by the rules of get a field trial from the factory, call
AppendGroup for all groups before doing anything else...

The only added functionality here, is that you can call SetForced after
completely initializing the FieldTrial so that we can let others attempt to
initialize it again, but the first initialization has precedence... Maybe
"forced" is not the appropriate terminology?

I'll try to update the comments to better explain the usage of this new
method...

[MAD, 2012-05-07 19:28:13]

On 2012/05/07 19:11:24, MAD wrote:
> Answers to questions, let me know if it's still not clear...
> 
> Will try to come up with better comment language to better describe this.
> 
> Thanks!
> 
> BYE
> MAD...
> 
>
https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
> File base/metrics/field_trial.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
> base/metrics/field_trial.cc:176: if (forced_)
> On 2012/05/07 18:58:01, jar wrote:
> > How does this handle "races?"  I'm curious about several folks trying ot do
> > forced on different threads (there is no apparent restriciton... and you
> mention
> > that other folks could have come sooner?).
> > 
> 
> From what I understand of this code, this is as racy as the rest of the
> FieldTrial class which doesn't protect at all against setting and/or usage
from
> different threads.
> 
> Other folks could have created this FieldTrial sooner in the same thread... I
> could add DCHECKs in all appropriate FieldTrial methods if you think we need
> that...
> 
>
https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
> base/metrics/field_trial.cc:184: group();
> On 2012/05/07 18:58:01, jar wrote:
> > Here again, how does this play in an async case?  For instance, could be
call
> > group() while someone else is still buildin the version up, and has not
> appended
> > all teh groups?
> > 
> > I'm very unclear about the (intended) semantics.
> 
> I'm not sure which "async case" you are talking about. This is all intended to
> happen from the UI thread at initialization time. The semantics have not
> changed, one must play by the rules of get a field trial from the factory,
call
> AppendGroup for all groups before doing anything else...
> 
> The only added functionality here, is that you can call SetForced after
> completely initializing the FieldTrial so that we can let others attempt to
> initialize it again, but the first initialization has precedence... Maybe
> "forced" is not the appropriate terminology?
> 
> I'll try to update the comments to better explain the usage of this new
> method...

How about this for the new wording for the header file comments?

[jar (doing other things), 2012-05-07 19:46:19]

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:176: if (forced_)
Maybe my memory is failing me, but the current (old?) contract with the
FieldTrial class was that only a single instance of a field trial (by a given
name) should be created. Most (all?) users of field trials sought to establish
the trials when the browser was single threaded, or when an applicable thread
(IO thread?) was first started up.  I think that precluded much "use before
construction" potential.  

Worst case, if I missed something, we could use a passive check for a "finalized
group status" when folks are creating histograms and such, so that there could
be no race to finalize (while a trial was being fleshed out).  Alternatively (if
there is an outstanding race), we could have transitioned to registering only
after finalized, and rely on the lock in the registration process to mediate any
races.

This new code is appearing to provide a second way to create and set
probabilities, and I'm not clear how that works.


On 2012/05/07 19:11:24, MAD wrote:
> On 2012/05/07 18:58:01, jar wrote:
> > How does this handle "races?"  I'm curious about several folks trying ot do
> > forced on different threads (there is no apparent restriciton... and you
> mention
> > that other folks could have come sooner?).
> > 
> 
> From what I understand of this code, this is as racy as the rest of the
> FieldTrial class which doesn't protect at all against setting and/or usage
from
> different threads.
> 
> Other folks could have created this FieldTrial sooner in the same thread... I
> could add DCHECKs in all appropriate FieldTrial methods if you think we need
> that...

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:184: group();
Perhaps if this is supposed to run on the UI thread, we could add an assert for
clarity.  It might also be nice to assert that this was run (perchance) before
we started up the other threads.

Note that most FieldTrial code can run on any thread, and uses the lock in the
Registration to mediate races.


On 2012/05/07 19:11:24, MAD wrote:
> On 2012/05/07 18:58:01, jar wrote:
> > Here again, how does this play in an async case?  For instance, could be
call
> > group() while someone else is still buildin the version up, and has not
> appended
> > all teh groups?
> > 
> > I'm very unclear about the (intended) semantics.
> 
> I'm not sure which "async case" you are talking about. This is all intended to
> happen from the UI thread at initialization time. The semantics have not
> changed, one must play by the rules of get a field trial from the factory,
call
> AppendGroup for all groups before doing anything else...
> 
> The only added functionality here, is that you can call SetForced after
> completely initializing the FieldTrial so that we can let others attempt to
> initialize it again, but the first initialization has precedence... Maybe
> "forced" is not the appropriate terminology?
> 
> I'll try to update the comments to better explain the usage of this new
> method...

[MAD, 2012-05-07 20:00:53]

More answers to more questions...

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:176: if (forced_)
On 2012/05/07 19:46:19, jar wrote:
> Maybe my memory is failing me, but the current (old?) contract with the
> FieldTrial class was that only a single instance of a field trial (by a given
> name) should be created. Most (all?) users of field trials sought to establish
> the trials when the browser was single threaded, or when an applicable thread
> (IO thread?) was first started up.  I think that precluded much "use before
> construction" potential.  
> 
> Worst case, if I missed something, we could use a passive check for a
"finalized
> group status" when folks are creating histograms and such, so that there could
> be no race to finalize (while a trial was being fleshed out).  Alternatively
(if
> there is an outstanding race), we could have transitioned to registering only
> after finalized, and rely on the lock in the registration process to mediate
any
> races.
> 
> This new code is appearing to provide a second way to create and set
> probabilities, and I'm not clear how that works.
> 
> 
> On 2012/05/07 19:11:24, MAD wrote:
> > On 2012/05/07 18:58:01, jar wrote:
> > > How does this handle "races?"  I'm curious about several folks trying ot
do
> > > forced on different threads (there is no apparent restriciton... and you
> > mention
> > > that other folks could have come sooner?).
> > > 
> > 
> > From what I understand of this code, this is as racy as the rest of the
> > FieldTrial class which doesn't protect at all against setting and/or usage
> from
> > different threads.
> > 
> > Other folks could have created this FieldTrial sooner in the same thread...
I
> > could add DCHECKs in all appropriate FieldTrial methods if you think we need
> > that...
> 

This contract of a single FieldTrial per name still holds, the difference
introduced with the concept of a forced field trial is that it might have
already been created before someone tries to create it, because we want to make
modifications to the hard coded values used to register the field trial (or
force a group choice in the case of the command line switch for debugging).

This new code still follows the same rules you describe, as they are followed by
the current set of consumers of FieldTrials, so this is why I saw no need to
more restrictions or validations for thread safety, raciness or
asynchronocities.

The goal remains the same as it was since the beginning of the Finch project, to
allow a server side control of FieldTrial to avoid the need of pushing new
binaries to make modifications to group sizes for example. So this new code
enables the setting of group sizes more than once, and let the first one win.

If you have suggestions of how else we can do this, I'm all open... Unless I
missed something, I see no more problems with this approach than what was
already available to control FieldTrials.

https://chromiumcodereview.appspot.com/10382018/diff/7/base/metrics/field_tri...
base/metrics/field_trial.cc:184: group();
On 2012/05/07 19:46:19, jar wrote:
> Perhaps if this is supposed to run on the UI thread, we could add an assert
for
> clarity.  It might also be nice to assert that this was run (perchance) before
> we started up the other threads.
> 
> Note that most FieldTrial code can run on any thread, and uses the lock in the
> Registration to mediate races.
> 
> 
> On 2012/05/07 19:11:24, MAD wrote:
> > On 2012/05/07 18:58:01, jar wrote:
> > > Here again, how does this play in an async case?  For instance, could be
> call
> > > group() while someone else is still buildin the version up, and has not
> > appended
> > > all teh groups?
> > > 
> > > I'm very unclear about the (intended) semantics.
> > 
> > I'm not sure which "async case" you are talking about. This is all intended
to
> > happen from the UI thread at initialization time. The semantics have not
> > changed, one must play by the rules of get a field trial from the factory,
> call
> > AppendGroup for all groups before doing anything else...
> > 
> > The only added functionality here, is that you can call SetForced after
> > completely initializing the FieldTrial so that we can let others attempt to
> > initialize it again, but the first initialization has precedence... Maybe
> > "forced" is not the appropriate terminology?
> > 
> > I'll try to update the comments to better explain the usage of this new
> > method...
> 

Unfortunately, we can access the BrowserThread class from here... :-(

I could add some sort of thread checker if you want and make sure we are called
on the same thread that created the FieldTrialList global instance?

[jar (doing other things), 2012-05-07 20:55:01]

I'm still a tad nervous... but we'll see.

LGTM

[commit-bot: I haz the power, 2012-05-07 21:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10382018/10002

[MAD, 2012-05-07 21:02:59]

Thanks!

On Mon, May 7, 2012 at 4:55 PM, <jar@chromium.org> wrote:

> I'm still a tad nervous... but we'll see.
>
> LGTM
>
>
https://chromiumcodereview.**appspot.com/10382018/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-05-07 22:48:28]

Change committed as 135748