Make a channel and time limited persistent PreRead experiment

Make a channel and time limited persistent PreRead experiment

Our previous PreRead experiment involved flipping a coin on every launch, but we wish to persist the coin-toss between launches. Additionally, this version of the field trial includes an expiry date and limits it to the canary and dev channels.

We are unable to use the FieldTrial mechanism due to PreRead occuring prior to chrome.dll being loaded. As such, we use the registry for persisting the coin-toss and environment variables for communicating between chrome.exe and chrome.dll.

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

Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=98388

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

[chrisha, 2011-08-23 13:57:46]

PTAL

[Sigurður Ásgeirsson, 2011-08-23 20:54:11]

As we discussed, this won't work for side by side installs. Thankfully, Carlos
pointed out that you can get the BrowserDistribution in the .exe, and that will
provide base registry paths for configuration, by the looks of it.
See e.g.
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/app/client_util.cc&exact....

[chrisha, 2011-08-25 17:20:34]

PTAL

[Sigurður Ásgeirsson, 2011-08-25 17:44:14]

sweet!

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:102: // Get the build time. This is copied from
base::FieldTrial::GetBuildTime.
MetricsLogBase::GetBuildTime() is a public static member, can you just invoke it
from here?

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:118: return channel == L"canary" || channel == L"dev";
looks like the canary channel name is "canary build", but can also be referred
to by "GoogleChromeSxSDistribution::ChannelName()". See 
http://codesearch.google.com/#h0RrPvyPu-c/chrome/app/breakpad_win.cc&exact_pa...

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:184: // mechanism as PreReading happens in chrome.exe
prior to loading
PreReading -> pre-reading?

Good comment.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:199: if (PreReadShouldRun()) {
nit: the code would look a little cleaner with this in a utility function, as
then you can do early return on failure branches.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:279: // 'GetRegistryPath()' was added in support of
PreRead experiment.
I don't think you particularly need to explain this.

http://codereview.chromium.org/7712014/diff/8002/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7712014/diff/8002/chrome/browser/browser_main....
chrome/browser/browser_main.cc:169: #include "base/win/registry.h"  // For
PreRead experiment.
I don't think you need this include?

[chrisha, 2011-08-25 19:39:17]

Addressed siggi's comments.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:102: // Get the build time. This is copied from
base::FieldTrial::GetBuildTime.
On 2011/08/25 17:44:14, Ruðrugis wrote:
> MetricsLogBase::GetBuildTime() is a public static member, can you just invoke
it
> from here?

Unfortunately this is in seconds-since-unix-epoch, which base::Time can't use.
And FieldTrial::GetBuildTime is private.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:118: return channel == L"canary" || channel == L"dev";
On 2011/08/25 17:44:14, Ruðrugis wrote:
> looks like the canary channel name is "canary build", but can also be referred
> to by "GoogleChromeSxSDistribution::ChannelName()". See 
>
http://codesearch.google.com/#h0RrPvyPu-c/chrome/app/breakpad_win.cc&exact_pa...

Looking at that file, the channel name is actually "canary", but it's definitely
smarter to refer to it directly via GoogleChromeSxSDistribution.

Done.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:184: // mechanism as PreReading happens in chrome.exe
prior to loading
On 2011/08/25 17:44:14, Ruðrugis wrote:
> PreReading -> pre-reading?
> 
> Good comment.

Done.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:199: if (PreReadShouldRun()) {
On 2011/08/25 17:44:14, Ruðrugis wrote:
> nit: the code would look a little cleaner with this in a utility function, as
> then you can do early return on failure branches.

Done.

http://codereview.chromium.org/7712014/diff/8002/chrome/app/client_util.cc#ne...
chrome/app/client_util.cc:279: // 'GetRegistryPath()' was added in support of
PreRead experiment.
On 2011/08/25 17:44:14, Ruðrugis wrote:
> I don't think you particularly need to explain this.

Done.

http://codereview.chromium.org/7712014/diff/8002/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7712014/diff/8002/chrome/browser/browser_main....
chrome/browser/browser_main.cc:169: #include "base/win/registry.h"  // For
PreRead experiment.
On 2011/08/25 17:44:14, Ruðrugis wrote:
> I don't think you need this include?

Leftover from before refactor.

Done.

[Sigurður Ásgeirsson, 2011-08-25 19:52:44]

lgtm

[commit-bot: I haz the power, 2011-08-25 21:56:21]

Change committed as 98319

[jar (doing other things), 2011-08-25 23:52:07]

Why did you want to persist the experiment state?  Are you expecting run-to-run
impact?  Flipping a coin each time, and not persisting the state, can generally
give you lower variance since it often allows a single user to run in all
possible experiment paths.

[jbates, 2011-08-26 01:15:43]

Why no try bots?

[chrisha, 2011-08-26 01:41:16]

On 2011/08/25 23:52:07, jar wrote:
> Why did you want to persist the experiment state?  Are you expecting
run-to-run
> impact?  Flipping a coin each time, and not persisting the state, can
generally
> give you lower variance since it often allows a single user to run in all
> possible experiment paths.

Unfortunately, the O/S loader optimizations are not stateless between
chrome.exe/chrome.dll launches, so we were seeing some noise from that. Making
pre-read persistent will allow the loader to see consistent behaviour of the
executable.

[chrisha, 2011-08-26 01:42:43]

On 2011/08/26 01:15:43, jbates wrote:
> Why no try bots?

I had been told that simply checking 'commit' was fine, as it launches try-bots
automatically and doesn't commit unless the try-bots pass. I'm more than happy
to run through try-bots beforehand though, if that's the standard.

[chrisha, 2011-08-26 03:17:57]

Broke the tree and reverted. New CL coming tomorrow morning.

http://codereview.chromium.org/7712014/diff/14001/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

http://codereview.chromium.org/7712014/diff/14001/chrome/app/client_util.cc#n...
chrome/app/client_util.cc:115: #if defined(GOOGLE_CHROME_BUILD)
The code in this block was not being tested, as there are no official try-bots.

http://codereview.chromium.org/7712014/diff/14001/chrome/app/client_util.cc#n...
chrome/app/client_util.cc:121: return channel ==
GoogleChromeSxSDistribution::GetChannel() ||
This is the offender. Should be ChannelName(), not GetChannel().

[chrisha, 2011-08-26 15:05:47]

Fixed the broken line of code in a GOOGLE_CHROME_BUILD guard, tested on an
official build on my local machine.

PTAL.

[jar (doing other things), 2011-08-26 16:48:19]

[Drive by commentary on your comment....]

I'm sorry you got the (incorrect) info that hitting commit was good enough.
 It absolutely is not.

You should first get all the try bots to pass your patch.  If the bots are
broken, you should send messages to the troupers (per comments in the
try-bot failure emails).  As experience grows, you can sometimes deduce that
a try bot failure was caused by an unrelated bustage in the tree (or
flakyness), but it is generally best to "try again" until you see green on
all platforms (at least once).

IMO, only if it is a relatively small, low risk patch, should you then hit
"commit" and forget about it.  In your first 10-20 commits, you should
probably not use this feature, as you'll probably not be a good judge of
just how easy it is to break the tree (I'm speaking from too much experience
;-) ).

In general, it is critical to be watching the tree when you land, so as to
respond to bustage *quickly* (i.e., monitor tree, be on IRC), provide
analysis of the situation, and potentially revert.  The sheriff is only the
backstop when a developer fails to do their job.  If you have to rely on
sheriffs to back you out, then you've messed up (and it is courteous to send
an apology to the sheriff, or the person that reverted, covering for your
error).

It is especially poor form to not run the try bots, and not get your code to
locally compile, before landing.  That can still happen to the best of us
when working on multiple patches in a tree, or when an uploaded patch
worked, and your current tree has a hosed file that gets pulled into a
submit...  No matter what the excuse, we all feel pretty lame when we bust
the tree in such a way... and need to work to not have it happen again (and
certainly not often).

YMMV,

Jim

p.s., When you land, if the build is broken, and it is NOT your fault, you
are still on the hook to help green up the tree, and/or revert the cause of
the bustage.  You are only off-the-hook when you change has landed, and the
tree has gone green.  As a result, try not to land unless you can be around
for about an hour to help with (un)related carnage.

On Thu, Aug 25, 2011 at 6:42 PM, <chrisha@chromium.org> wrote:

> On 2011/08/26 01:15:43, jbates wrote:
>
>> Why no try bots?
>>
>
> I had been told that simply checking 'commit' was fine, as it launches
> try-bots
> automatically and doesn't commit unless the try-bots pass. I'm more than
> happy
> to run through try-bots beforehand though, if that's the standard.
>
>
>
http://codereview.chromium.**org/7712014/<http://codereview.chromium.org/7712...
>

[Sigurður Ásgeirsson, 2011-08-26 16:54:05]

lgtm

[chrisha, 2011-08-26 19:05:26]

Thanks, committing via the commit queue.

NOTE: The linux try bot seems flaky. It crashed on an unrelated unit-test.

[commit-bot: I haz the power, 2011-08-26 20:47:05]

Change committed as 98482