Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7508034:
Make a pre-read A/B field-trial (Closed)
|
Created:
9 years, 4 months ago by chrisha Modified:
9 years, 4 months ago CC:
chromium-reviews, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMake a pre-read A/B field-trial
This CL disables/enables the Windows pre-read optimization based on a client-side coin-toss. The base::FieldTrial mechanism has not been used as it is not available until *after* chrome.dll is loaded, and this needs to occur before.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95579
Patch Set 1 : '' #
Total comments: 16
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #
Total comments: 2
Patch Set 4 : '' #
Total comments: 3
Patch Set 5 : '' #
Total comments: 4
Patch Set 6 : '' #
Total comments: 2
Patch Set 7 : '' #
Messages
Total messages: 14 (0 generated)
PTAL.
This only big issue is the privacy angle. See comments below. http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:154: if (key.WriteValue(kPreReadExperimentKey, coin_toss) == 0) It is undesirable to save a random number into the registry, as this tags a browser with a unique ID. Privacy folks would not like it, despite the fact that is is not being used as a unique ID. It would be preferable to save a boolean result of the compare with a threshold. IF you plan on changing the threshold, you could save that as well, and then you could for example re-toss a coin when a different threshold is observed. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2046: uma_name = "Startup.BrowserOpenTabs_PreReadDisabled"; personal style nit: This logic is correct and OK. My preference to avoid typos is to list the prefix once, and then optionally add one of several postfix strings. For example: char* uma_name = "Startup.BrowserOpenTabs_PreRead"; uma_name += (pre_read == 0): "Disabled" : "Enabled".; http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2047: UMA_HISTOGRAM_MEDIUM_TIMES(uma_name, elapsed_from_browser_open_start); When performing detailed experiments, I've often found it valuable to have a full 100 buckets. I'd also sadly predict that you want a longer cutoff in your tail (you'll get too many samples in your top bucket otherwise). I'd suggest: UMA_HISTOGRAM_CUSTOM_TIMES(uma_name, elapsed_from_browser_open_start, base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromHours(1), 100) http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:109: uma_name = "Startup.BrowserMessageLoopStartTime_PreReadDisabled"; personal preference nit: Same comment about refactoring as made in browser_main.cc to avoid retyping the bulk of the histogram name. (p.s., typos in these names have been historically bothersome... and sadly common)
nice! http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:148: const wchar_t kPreReadExperimentKey[] = L"PreReadExperiment"; nit: static http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:154: if (key.WriteValue(kPreReadExperimentKey, coin_toss) == 0) On 2011/08/03 21:24:34, jar wrote: > It is undesirable to save a random number into the registry, as this tags a > browser with a unique ID. Privacy folks would not like it, despite the fact > that is is not being used as a unique ID. > > It would be preferable to save a boolean result of the compare with a threshold. > IF you plan on changing the threshold, you could save that as well, and then > you could for example re-toss a coin when a different threshold is observed. The range of the coin_toss here is 0 to 1. It would be useful to assert on this as documentation, e.g. DCHECK(coin_toss == 0 || coin_toss == 1); http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2038: base::win::RegKey key(HKEY_CURRENT_USER, L"Software\\Google\\ChromeFrame", as we discussed, we'd want to test the PreRead key as well, so that we can correctly bucket chrome.dlls that are loaded by older chrome.exes. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:102: base::win::RegKey key(HKEY_CURRENT_USER, L"Software\\Google\\ChromeFrame", ditto: is there a good common place to put this reading?
I address jar@ and siggi@'s comments. Additionally, there was a rework of how the experiment group is determined by chrome.dll to eliminate possible version conflicts between chrome.exe and chrome.dll. PTAnotherL. http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:148: const wchar_t kPreReadExperimentKey[] = L"PreReadExperiment"; On 2011/08/04 13:35:32, Ruðrugis wrote: > nit: static Done. http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:154: if (key.WriteValue(kPreReadExperimentKey, coin_toss) == 0) On 2011/08/04 13:35:32, Ruðrugis wrote: > On 2011/08/03 21:24:34, jar wrote: > > It is undesirable to save a random number into the registry, as this tags a > > browser with a unique ID. Privacy folks would not like it, despite the fact > > that is is not being used as a unique ID. > > > > It would be preferable to save a boolean result of the compare with a > threshold. > > IF you plan on changing the threshold, you could save that as well, and then > > you could for example re-toss a coin when a different threshold is observed. > > The range of the coin_toss here is 0 to 1. It would be useful to assert on this > as documentation, e.g. DCHECK(coin_toss == 0 || coin_toss == 1); Done. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2038: base::win::RegKey key(HKEY_CURRENT_USER, L"Software\\Google\\ChromeFrame", On 2011/08/04 13:35:32, Ruðrugis wrote: > as we discussed, we'd want to test the PreRead key as well, so that we can > correctly bucket chrome.dlls that are loaded by older chrome.exes. As we discussed, there's the potential for an impedance mismatch between chrome.exe and chrome.dll with this approach. If an experiment-enabled chrome.exe runs and sets PreReadExperiment, and then chrome.dll is launched by an experiment-disabled chrome.exe, then chrome.dll will still report experiment results even though it hasn't taken place. This can causes problems in the leading and trailing edges of our experiment, as chrome.dll tends to update faster and sooner than chrome.exe. I've moved to using the registry to make the coin-toss result persistent, but I communicate that result via side channel directly from the running chrome.exe to the running chrome.dll (using environment variables). This ensures that experiment results are only reported when the experiment runs. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2046: uma_name = "Startup.BrowserOpenTabs_PreReadDisabled"; On 2011/08/03 21:24:34, jar wrote: > personal style nit: > > This logic is correct and OK. My preference to avoid typos is to list the > prefix once, and then optionally add one of several postfix strings. For > example: > char* uma_name = "Startup.BrowserOpenTabs_PreRead"; > uma_name += (pre_read == 0): "Disabled" : "Enabled".; > Done. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main.... chrome/browser/browser_main.cc:2047: UMA_HISTOGRAM_MEDIUM_TIMES(uma_name, elapsed_from_browser_open_start); On 2011/08/03 21:24:34, jar wrote: > When performing detailed experiments, I've often found it valuable to have a > full 100 buckets. I'd also sadly predict that you want a longer cutoff in your > tail (you'll get too many samples in your top bucket otherwise). > I'd suggest: > UMA_HISTOGRAM_CUSTOM_TIMES(uma_name, elapsed_from_browser_open_start, > base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromHours(1), 100) Done. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:102: base::win::RegKey key(HKEY_CURRENT_USER, L"Software\\Google\\ChromeFrame", On 2011/08/04 13:35:32, Ruðrugis wrote: > ditto: is there a good common place to put this reading? I moved the common functionality to a function declared in browser_main_win.h. http://codereview.chromium.org/7508034/diff/4001/chrome/browser/browser_main_... chrome/browser/browser_main_win.cc:109: uma_name = "Startup.BrowserMessageLoopStartTime_PreReadDisabled"; On 2011/08/03 21:24:34, jar wrote: > personal preference nit: Same comment about refactoring as made in > browser_main.cc to avoid retyping the bulk of the histogram name. > > (p.s., typos in these names have been historically bothersome... and sadly > common) Done.
nice! http://codereview.chromium.org/7508034/diff/13003/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/13003/chrome/app/client_util.cc#n... chrome/app/client_util.cc:168: env->SetVar("CHROME_PRE_READ_EXPERIMENT", as we discussed, this only runs if the regkey exists, but we don't actually need the coin toss to persist. So, better to move the toss out of this block and toss every time. http://codereview.chromium.org/7508034/diff/13003/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/13003/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:285: UMA_HISTOGRAM_CUSTOM_TIMES(name, time, kMin, kMax, kBuckets); Shouldn't this be mutually exclusive to recording experiment results? As-is, we'll taint this histogram?
Side-stepped the registry entirely, and moved to a coin-toss that occurs at every chrome.exe launch. PTAnotherL http://codereview.chromium.org/7508034/diff/13003/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/13003/chrome/app/client_util.cc#n... chrome/app/client_util.cc:168: env->SetVar("CHROME_PRE_READ_EXPERIMENT", On 2011/08/04 15:18:38, Ruðrugis wrote: > as we discussed, this only runs if the regkey exists, but we don't actually need > the coin toss to persist. So, better to move the toss out of this block and toss > every time. Done. http://codereview.chromium.org/7508034/diff/13003/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/13003/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:285: UMA_HISTOGRAM_CUSTOM_TIMES(name, time, kMin, kMax, kBuckets); On 2011/08/04 15:18:38, Ruðrugis wrote: > Shouldn't this be mutually exclusive to recording experiment results? > As-is, we'll taint this histogram? The documentation of field_trial.h suggests that convention is always to populate the original histogram with data from all users, regardless of the experiment group they belong to.
lgtm with one last nit http://codereview.chromium.org/7508034/diff/13005/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/13005/chrome/app/client_util.cc#n... chrome/app/client_util.cc:151: // We communicate the coin-toss result via a side-channel This LGTM, except that we probably ought to file a bug against removing this before the next branch point. Can I ask you to file the bug and reference it here?
http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/4001/chrome/app/client_util.cc#ne... chrome/app/client_util.cc:154: if (key.WriteValue(kPreReadExperimentKey, coin_toss) == 0) My apologies for my mis-read. Thanks for the assert. On 2011/08/04 13:35:32, Ruðrugis wrote: > On 2011/08/03 21:24:34, jar wrote: > > It is undesirable to save a random number into the registry, as this tags a > > browser with a unique ID. Privacy folks would not like it, despite the fact > > that is is not being used as a unique ID. > > > > It would be preferable to save a boolean result of the compare with a > threshold. > > IF you plan on changing the threshold, you could save that as well, and then > > you could for example re-toss a coin when a different threshold is observed. > > The range of the coin_toss here is 0 to 1. It would be useful to assert on this > as documentation, e.g. DCHECK(coin_toss == 0 || coin_toss == 1); http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:285: UMA_HISTOGRAM_CUSTOM_TIMES(name, time, kMin, kMax, kBuckets); Am I mistaken, or is this function called with two distinct values of |name|? This macro was not intended to be refactored this way, and expects a constant name. I *think* you call this function with two distinct histogram names, and the macro should then assert out. The "problem" is that the macro contains a static definition, as most users will call it many times... and performance dictates finding the named histogram only once.
Alright... hopefully the last iteration. Tested and doing what it is supposed to be doing. http://codereview.chromium.org/7508034/diff/13005/chrome/app/client_util.cc File chrome/app/client_util.cc (right): http://codereview.chromium.org/7508034/diff/13005/chrome/app/client_util.cc#n... chrome/app/client_util.cc:151: // We communicate the coin-toss result via a side-channel On 2011/08/04 15:32:00, Ruðrugis wrote: > This LGTM, except that we probably ought to file a bug against removing this > before the next branch point. Can I ask you to file the bug and reference it > here? I added a comment to that effect here. http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:285: UMA_HISTOGRAM_CUSTOM_TIMES(name, time, kMin, kMax, kBuckets); On 2011/08/04 18:34:28, jar wrote: > Am I mistaken, or is this function called with two distinct values of |name|? > > This macro was not intended to be refactored this way, and expects a constant > name. I *think* you call this function with two distinct histogram names, and > the macro should then assert out. The "problem" is that the macro contains a > static definition, as most users will call it many times... and performance > dictates finding the named histogram only once. No, you're not mistaken, that's exactly what its doing. Not having used UMA before I wasn't aware of the behaviour of the macros. But, when running tests on my local machine I was only seeing a single Startup.* histogram with 2 samples (first one in wins)! This has been fixed by calling base::Histogram::FactoryTimeGet directly.
lgtm with a last couple of nits. http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:277: void AddPreReadHistogramTime(const char* name, base::TimeDelta time) { can this function be static? http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:278: static const base::TimeDelta kMin(base::TimeDelta::FromMilliseconds(1)); These locals ought to be non-static per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...
I don't have the ability to commit to chrome. Care to take care of this for me siggi? http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:277: void AddPreReadHistogramTime(const char* name, base::TimeDelta time) { On 2011/08/04 18:57:52, Ruðrugis wrote: > can this function be static? Done. http://codereview.chromium.org/7508034/diff/13009/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:278: static const base::TimeDelta kMin(base::TimeDelta::FromMilliseconds(1)); On 2011/08/04 18:57:52, Ruðrugis wrote: > These locals ought to be non-static per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... Done.
LGTM See nit below. http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/14008/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:285: UMA_HISTOGRAM_CUSTOM_TIMES(name, time, kMin, kMax, kBuckets); Be sure to run under Debug, as it would have diagnosed the problem via a DCHECK for you. On 2011/08/04 18:50:33, chrisha wrote: > On 2011/08/04 18:34:28, jar wrote: > > Am I mistaken, or is this function called with two distinct values of |name|? > > > > This macro was not intended to be refactored this way, and expects a constant > > name. I *think* you call this function with two distinct histogram names, and > > the macro should then assert out. The "problem" is that the macro contains a > > static definition, as most users will call it many times... and performance > > dictates finding the named histogram only once. > > No, you're not mistaken, that's exactly what its doing. > > Not having used UMA before I wasn't aware of the behaviour of the macros. But, > when running tests on my local machine I was only seeing a single Startup.* > histogram with 2 samples (first one in wins)! This has been fixed by calling > base::Histogram::FactoryTimeGet directly. http://codereview.chromium.org/7508034/diff/14016/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:286: nit: I always think it is strange to DCHECK (or CHECK) a pointer, and then dereference it, as the deref will crash both debug and release builds. If you really like the DCHECK, use either: DCHECK(counter); or DCHECK_NE(counter, NULL);
Thanks, committing. http://codereview.chromium.org/7508034/diff/14016/chrome/browser/browser_main... File chrome/browser/browser_main_win.cc (right): http://codereview.chromium.org/7508034/diff/14016/chrome/browser/browser_main... chrome/browser/browser_main_win.cc:286: DCHECK(counter != NULL); On 2011/08/04 19:11:03, jar wrote: > nit: I always think it is strange to DCHECK (or CHECK) a pointer, and then > dereference it, as the deref will crash both debug and release builds. > > If you really like the DCHECK, use either: > DCHECK(counter); > or > DCHECK_NE(counter, NULL); Removed.
Change committed as 95579 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
