Re-enable pre-read experiment as a finch field trial.

chrome/app/client_util.cc

Index: chrome/app/client_util.cc
diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc
index 0d1c52cb3a433cdd80f85c38e5d36d5856891d6c..2ac1d9a523f2483aa8966708dbdacbad0d59f75f 100644
--- a/chrome/app/client_util.cc
+++ b/chrome/app/client_util.cc
@@ -11,6 +11,8 @@
 #include "base/file_version_info.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/rand_util.h"  // For PreRead experiment.
+#include "base/sha1.h"  // For PreRead experiment.
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -37,6 +39,107 @@ typedef int (*DLL_MAIN)(HINSTANCE, sandbox::SandboxInterfaceInfo*);
 
 typedef void (*RelaunchChromeBrowserWithNewCommandLineIfNeededFunc)();
 
+bool PreReadExpirementHasExpired() {
+  static const int kPreReadExpiryYear = 2014;
+  static const int kPreReadExpiryMonth = 7;
+  static const int kPreReadExpiryDay = 1;
+  static const char kBuildTimeStr[] = __DATE__ " " __TIME__;
+
+  // Get the timestamp of the build.
+  base::Time build_time;
+  bool result = base::Time::FromString(kBuildTimeStr, &build_time);
+  DCHECK(result);
+
+  // Get the timestamp at which the experiment expires.
+  base::Time::Exploded exploded = {0};
+  exploded.year = kPreReadExpiryYear;
+  exploded.month = kPreReadExpiryMonth;
+  exploded.day_of_month = kPreReadExpiryDay;
+  base::Time expiration_time = base::Time::FromLocalExploded(exploded);
+
+  // Return true if the experiment is expired.
+  return (build_time > expiration_time);

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Nit: No ()'s needed.

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+}
+
+void GetRandomPopulationAndGroup(double* population, double* group) {

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Add a comment explaining what this does (and the output params).

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+  DCHECK(population != NULL);
+  DCHECK(group != NULL);
+
+  // Use the metrics id for the user as stable pseudo-random input to a hash.
+  base::string16 key;
+  GoogleUpdateSettings::GetMetricsId(&key);

[Roger McFarlane (Chromium), 2013/08/29 15:28:37]

Fall back to something like the MAC address is the metrics_id is empty?

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Hmm, it is unfortunate that you need this to be persistent. Normally, this is
done by using the "low entropy source" value, but I don't think it's available
here, since unlike client id (aka GetMetricsId()) we don't also mirror it in
registry - it's only in local state.

Since we're not getting UMA metrics for non-UMA users (that don't have a metrics
id), we could either make it so the dice roll is _not_ persistent for those
users (since we won't look at their stats), or we can go back to the previous
approach of only giving the experiment to UMA users.

Neither is particularly great - what are your thoughts on dice-rolling non-UMA
users to a different group every time? Will that potentially give them a poor
user experience due to bad Windows preread caching behaviour or will it mostly
be okay? What do you think?

(The other alternative is to expose the low entropy source in the registry, but
that seems like a big change just for this.)

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Per your suggestion: if I don't have the metrics id, I've added a bit of code to
the dice each time.

@cpu: how much do you think this will confuse the prefetcher? Do you think this
might degrade the performance beyond that of any group in the experiment?

+
+  // To interpret the key as a random number we hash it and intepret the first

[chrisha, 2013/08/29 15:52:19]

interpret*

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+  // 8 bytes of it as a unit-interval representing a die-toss for being in the
+  // experiment population and the second 8 bytes as a die-toss for being in
+  // various experiment groups.
+  unsigned char sha1_hash[base::kSHA1Length];
+  base::SHA1HashBytes(
+      reinterpret_cast<const unsigned char*>(key.c_str()),
+      key.size() * sizeof(key[0]),
+      sha1_hash);
+  COMPILE_ASSERT(2 * sizeof(uint64) < sizeof(sha1_hash), need_more_data);
+  const uint64* population_bits = reinterpret_cast<uint64*>(&sha1_hash[0]);
+  const uint64* group_bits = population_bits + 1;

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

I think it would be slightly clearer to keep a single ptr var and just index it
at 0 and at 1 below.

i.e. 

const uint64* random_bits = reinterpret_cast<uint64*>(&sha1_hash[0]);

*population = base::BitsToOpenEndedUnitInterval(random_bits[0]);
*group = base::BitsToOpenEndedUnitInterval(random_bits[1]);

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+
+  // Convert the bits into unit-intervals and return.
+  *population = base::BitsToOpenEndedUnitInterval(*population_bits);
+  *group = base::BitsToOpenEndedUnitInterval(*group_bits);
+}
+
+uint8 GetPreReadPercentage() {
+  // By default use the old behaviour: read 100%.
+  static const uint8 kDefaultPercentage = 100;
+  static const char kDefaultFormatStr[] = "%d%%-Default";
+  static const char kControlFormatStr[] = "%d%%-Control";
+  static const char kGroupFormatStr[] = "%d%%";
+
+  COMPILE_ASSERT(kDefaultPercentage <= 100, default_percentage_too_large);
+  COMPILE_ASSERT(kDefaultPercentage % 5 == 0, default_percentage_not_mult_5);
+
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+
+  // If the expirement has expired use the default pre-read level.

[chrisha, 2013/08/29 15:52:19]

experiment*

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+  if (PreReadExpirementHasExpired()) {
+    env->SetVar(chrome::kPreReadEnvironmentVariable,
+                base::StringPrintf(kDefaultFormatStr, kDefaultPercentage));
+    return kDefaultPercentage;
+  }
+
+  // Roll the dice to determine if this user is in the experiment and if so,
+  // in which experimental group.
+  double population = 0.0;
+  double group = 0.0;
+  GetRandomPopulationAndGroup(&population, &group);
+
+  // We limit experiment populations to 1% of the Stable and 10% of the Beta
+  // and Dev channels.
+  const string16 channel(GoogleUpdateSettings::GetChromeChannel(
+      GoogleUpdateSettings::IsSystemInstall()));
+  if ((channel == installer::kChromeChannelStable && population > 0.01) ||
+      ((channel == installer::kChromeChannelBeta ||

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Instead of checking for the other channels explicitly, just check != stable - so
that e.g. canary is also affected (this way, if it causes problems in canary,
you'll catch it early). I suggest just assigning a threshold value based on the
channel and comparing against that, e.g.:

double threshold = (channel == Installer::kChromeChannelStable) ? 0.1 : 0.01;
if (population > threshold)
  return kDefaultPercentage;

+        channel == installer::kChromeChannelDev) && population > 0.10)) {
+    env->SetVar(chrome::kPreReadEnvironmentVariable,
+                base::StringPrintf(kDefaultFormatStr, kDefaultPercentage));
+    return kDefaultPercentage;
+  }
+
+  // We divide the experiment population into groups pre-reading at 5 percent
+  // increments. This is 21 groups of 5 -- to include the range [100, 105) --
+  // rounded down to the nearest 5.
+  uint8 percentage = static_cast<uint8>(group * 21.0) * 5;

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Use a size_t instead.

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+  DCHECK_GE(100, percentage);

[chrisha, 2013/08/29 15:52:19]

100u?

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

For macros like _GE, the literal doesn't have to be the first arg and in this
case it's more readable to do *_LE(percentage, 100).

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+  DCHECK_EQ(0, percentage % 5);

[chrisha, 2013/08/29 15:52:19]

0u?

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+
+  const char* format_str =
+    (percentage == kDefaultPercentage) ? kControlFormatStr : kGroupFormatStr;
+
+  env->SetVar(chrome::kPreReadEnvironmentVariable,
+              base::StringPrintf(format_str, percentage));

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

I think it would be cleaner for this function to not set |env| state but instead
just return the percentage and associated group string (or just the percentage
and generate the string from that). Then, you only need a single SetVar() call
on the result at the callsite below.

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

I've refactored it a bit. Is this better?

I'd rather put the experimental vagaries in close proximity to one another.

+
+  return percentage;
+}
+
 // Expects that |dir| has a trailing backslash. |dir| is modified so it
 // contains the full path that was tried. Caller must check for the return
 // value not being null to determine if this path contains a valid dll.
@@ -57,9 +160,9 @@ HMODULE LoadChromeWithDirectory(string16* dir) {
   // performs slightly better than 100%. On XP, pre-read is generally a
   // performance loss.
   if (!cmd_line.HasSwitch(switches::kProcessType)) {
-    const size_t kStepSize = 1024 * 1024;
-    uint8 percent = base::win::GetVersion() > base::win::VERSION_XP ? 25 : 0;
-    ImagePreReader::PartialPreReadImage(dir->c_str(), percent, kStepSize);
+    static const size_t kStepSize = 1024 * 1024;

[Alexei Svitkine (slow), 2013/08/29 17:44:05]

Nit: No need for static.

[Roger McFarlane (Chromium), 2013/08/30 21:27:37]

Done.

+    uint8 percentage = GetPreReadPercentage();
+    ImagePreReader::PartialPreReadImage(dir->c_str(), percentage, kStepSize);
   }
 #endif