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

base/metrics/field_trial.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/metrics/field_trial.h"
 
 #include "base/build_time.h"
 #include "base/logging.h"
 #include "base/rand_util.h"
 #include "base/sha1.h"
@@ skipping 152 matching lines @@
   big_string.append(1, kHistogramFieldTrialSeparator);
   return big_string.append(FieldTrialList::FindFullName(trial_name));
 }
 
 // static
 void FieldTrial::EnableBenchmarking() {
   DCHECK_EQ(0u, FieldTrialList::GetFieldTrialCount());
   enable_benchmarking_ = true;
 }
 
+void FieldTrial::SetForced() {
+  // We might have been forced before (e.g., by CreateFieldTrial) and it's
+  // first come first served, e.g., command line switch has precedence.
+  if (forced_)

[jar (doing other things), 2012/05/07 18:58:01]

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?).

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

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...

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

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:

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

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.

+    return;
+  // 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.
+  DCHECK_GT(next_group_number_, kDefaultGroupNumber + 1);
+
+  // And we must finalize the group choice before we mark ourselves as forced.
+  group();

[jar (doing other things), 2012/05/07 18:58:01]

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]

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...

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

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:

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

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?

+  forced_ = true;
+}
+
 FieldTrial::~FieldTrial() {}
 
 void FieldTrial::SetGroupChoice(const std::string& name, int number) {
   group_ = number;
   if (name.empty())
     StringAppendF(&group_name_, "%d", group_);
   else
     group_name_ = name;
 }
 
@@ skipping 275 matching lines @@
     used_without_global_ = true;
     return;
   }
   AutoLock auto_lock(global_->lock_);
   DCHECK(!global_->PreLockedFind(trial->name()));
   trial->AddRef();
   global_->registered_[trial->name()] = trial;
 }
 
 }  // namespace base