Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23534009:
Re-enable pre-read experiment as a finch field trial. (Closed)
|
Created:
7 years, 3 months ago by Roger McFarlane (Chromium) Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRe-enable pre-read experiment as a finch field trial.
This CL revives an expired pre-read experiment as a Finch field trial. About 2 months ago the expired experiment, and its fallback behaviour, were removed and there was a subsequent performance regression.
R=cpu, asvitkine
BUG=245435, 280721
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223070
Patch Set 1 #
Total comments: 8
Patch Set 2 : Declare all of the experiment groups... and other refactorings. #
Total comments: 30
Patch Set 3 : Address comments from chrisha and asvitkine. #
Total comments: 10
Patch Set 4 : Address Alexei's comments #
Total comments: 2
Patch Set 5 : Fix a nit #Patch Set 6 : rebase #
Total comments: 12
Patch Set 7 : Fix nits and rebase #Patch Set 8 : Expose pre-read environment constant on all platforms #Patch Set 9 : uint8 to size_t for win64 compile #Patch Set 10 : fix comment and rebase #
Messages
Total messages: 27 (0 generated)
PTAL @asvitkine, are there naming conventions for the trial and group names that i'm violating?
https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/app/client_util... chrome/app/client_util.cc:78: // If this user isn't reporting metrics, use the default pre-read leve. The general rule is to avoid giving different behaviour to users depending on whether UMA is enabled. Because if you do, then it may encourage people to opt-out of UMA to change the behaviour, which we really don't want. As such, you should make everyone eligible for your experiment. https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/app/client_util... chrome/app/client_util.cc:90: reinterpret_cast<const unsigned char*>(metrics_id.c_str()), Do the results need to be persistent? i.e. does the fact that my last startup was done via pre-read cause a problem if my next startup isn't or vice versa? If they're independent, I suggest just tossing a coin rather than hashing a persistent value to determine if the user should be in the trial. You can do this via a simple call to base::RandDouble() and compare that result. https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:721: if (env->GetVar(chrome::kPreReadEnvironmentVariable, &pre_read_group)) { We have a script that parses source code to extract field trial names and group names, which is necessary to make their hashes known to the dashboard. If you use a variable (rather than a string literal) for the group name here, the script won't find those. You can fix this by invoking those calls explicitly here, e.g.: if (pre_read_group == "foo") base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, "foo"); else if (pre_read_group == "bar") base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, "bar"); ... etc. Or you can submit a CL to google3 to add your extra strings to the list the script knows about that can't be extracted from source. https://chromiumcodereview.appspot.com/23534009/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:723: base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, pre_read_group); You need to call group() on returned FieldTrial* object, else the field trial will be considered unused and not be reported in upstream metrics.
I want to hold on doing this until Roger's other pre-read CL lands and I can merge into m30. I want the data from that to figure out if indeed an experiment is warranted.
On 2013/08/28 17:57:07, cpu wrote: > I want to hold on doing this until Roger's other pre-read CL lands and I can > merge into m30. I want the data from that to figure out if indeed an experiment > is warranted. The other CL should definitely land first. That said, the other CL will (at best) fix the regression. It won't tell us if that performance level is optimal. We have some (now outdated) experimental results which suggest it isn't. Reviving the experiment is the right way to answer the question for current chromen.
Another look? https://codereview.chromium.org/23534009/diff/1/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/1/chrome/app/client_util.cc#new... chrome/app/client_util.cc:78: // If this user isn't reporting metrics, use the default pre-read leve. On 2013/08/28 17:30:50, Alexei Svitkine wrote: > The general rule is to avoid giving different behaviour to users depending on > whether UMA is enabled. Because if you do, then it may encourage people to > opt-out of UMA to change the behaviour, which we really don't want. > > As such, you should make everyone eligible for your experiment. Fair enough. I copied this "opt-out" logic from the original experiment. Is it a reasonable concern that metrics_id might be empty? If so, should I be using something else as the random data source? https://codereview.chromium.org/23534009/diff/1/chrome/app/client_util.cc#new... chrome/app/client_util.cc:90: reinterpret_cast<const unsigned char*>(metrics_id.c_str()), On 2013/08/28 17:30:50, Alexei Svitkine wrote: > Do the results need to be persistent? > > i.e. does the fact that my last startup was done via pre-read cause a problem if > my next startup isn't or vice versa? > > If they're independent, I suggest just tossing a coin rather than hashing a > persistent value to determine if the user should be in the trial. You can do > this via a simple call to base::RandDouble() and compare that result. Yes, they need to be persistent. The OS pre-fetcher is learning the access patterns of the binary; so, from run to run, the pre-read should be consistent, as this will be a factor affecting the times being meassured. https://codereview.chromium.org/23534009/diff/1/chrome/browser/chrome_browser... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/1/chrome/browser/chrome_browser... chrome/browser/chrome_browser_main.cc:721: if (env->GetVar(chrome::kPreReadEnvironmentVariable, &pre_read_group)) { On 2013/08/28 17:30:50, Alexei Svitkine wrote: > We have a script that parses source code to extract field trial names and group > names, which is necessary to make their hashes known to the dashboard. > > If you use a variable (rather than a string literal) for the group name here, > the script won't find those. > > You can fix this by invoking those calls explicitly here, e.g.: > > if (pre_read_group == "foo") > base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, "foo"); > else if (pre_read_group == "bar") > base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, "bar"); > > ... etc. > > Or you can submit a CL to google3 to add your extra strings to the list the > script knows about that can't be extracted from source. I've pulled this out to chrome_browser_field_trials_desktop.cc. PTAL? https://codereview.chromium.org/23534009/diff/1/chrome/browser/chrome_browser... chrome/browser/chrome_browser_main.cc:723: base::FieldTrialList::CreateFieldTrial(kPreReadTrialName, pre_read_group); On 2013/08/28 17:30:50, Alexei Svitkine wrote: > You need to call group() on returned FieldTrial* object, else the field trial > will be considered unused and not be reported in upstream metrics. Having created a setup function for the experiment this should now be happening implicitly, IIUC. Right? https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:70: GoogleUpdateSettings::GetMetricsId(&key); Fall back to something like the MAC address is the metrics_id is empty? https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:717: AddPreReadHistogramTime(name, time); As this function has now been reduced to nothing, should it be clobbered and the call-sites updated to directly call AddPreREadHistogramTime()? Could the call-sites even just be updated to use UMA_HISTOGRAM_*?
The overall approach looks good to me, but I'm not a Finch expert so I won't comment on the experiment design. And other than a few nits, I don't have much to say. https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:72: // To interpret the key as a random number we hash it and intepret the first interpret* https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:102: // If the expirement has expired use the default pre-read level. experiment* https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:131: DCHECK_GE(100, percentage); 100u? https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:132: DCHECK_EQ(0, percentage % 5); 0u?
https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:61: return (build_time > expiration_time); Nit: No ()'s needed. https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:64: void GetRandomPopulationAndGroup(double* population, double* group) { Add a comment explaining what this does (and the output params). https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:70: GoogleUpdateSettings::GetMetricsId(&key); On 2013/08/29 15:28:37, Roger McFarlane (Chromium) wrote: > Fall back to something like the MAC address is the metrics_id is empty? 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.) https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:83: const uint64* group_bits = population_bits + 1; 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]); https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:120: ((channel == installer::kChromeChannelBeta || 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; https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:130: uint8 percentage = static_cast<uint8>(group * 21.0) * 5; Use a size_t instead. https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:131: DCHECK_GE(100, percentage); 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). https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:138: base::StringPrintf(format_str, percentage)); 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. https://codereview.chromium.org/23534009/diff/8001/chrome/app/client_util.cc#... chrome/app/client_util.cc:163: static const size_t kStepSize = 1024 * 1024; Nit: No need for static. https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_field_trials_desktop.cc:131: #if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD) Is there a reason this should only be activated in a GOOGLE_CHROME_BUILD? We don't generally have that restriction on other field trials. https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:717: AddPreReadHistogramTime(name, time); On 2013/08/29 15:28:37, Roger McFarlane (Chromium) wrote: > As this function has now been reduced to nothing, should it be clobbered and the > call-sites updated to directly call AddPreREadHistogramTime()? Could the > call-sites even just be updated to use UMA_HISTOGRAM_*? I agree, it should just be replaced with an equivalent UMA_HISTOGRAM_*() macro call.
Thanks. Another look. +thakis for OWNERS. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... File chrome/app/client_util.cc (right): https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:61: return (build_time > expiration_time); On 2013/08/29 17:44:05, Alexei Svitkine wrote: > Nit: No ()'s needed. Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:64: void GetRandomPopulationAndGroup(double* population, double* group) { On 2013/08/29 17:44:05, Alexei Svitkine wrote: > Add a comment explaining what this does (and the output params). Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:70: GoogleUpdateSettings::GetMetricsId(&key); On 2013/08/29 17:44:05, Alexei Svitkine wrote: > On 2013/08/29 15:28:37, Roger McFarlane (Chromium) wrote: > > Fall back to something like the MAC address is the metrics_id is empty? > > 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.) 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? https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:72: // To interpret the key as a random number we hash it and intepret the first On 2013/08/29 15:52:19, chrisha wrote: > interpret* Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:83: const uint64* group_bits = population_bits + 1; On 2013/08/29 17:44:05, Alexei Svitkine wrote: > 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]); Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:102: // If the expirement has expired use the default pre-read level. On 2013/08/29 15:52:19, chrisha wrote: > experiment* Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:130: uint8 percentage = static_cast<uint8>(group * 21.0) * 5; On 2013/08/29 17:44:05, Alexei Svitkine wrote: > Use a size_t instead. Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:131: DCHECK_GE(100, percentage); On 2013/08/29 15:52:19, chrisha wrote: > 100u? Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:132: DCHECK_EQ(0, percentage % 5); On 2013/08/29 15:52:19, chrisha wrote: > 0u? Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:138: base::StringPrintf(format_str, percentage)); On 2013/08/29 17:44:05, Alexei Svitkine wrote: > 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. I've refactored it a bit. Is this better? I'd rather put the experimental vagaries in close proximity to one another. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/app/client_u... chrome/app/client_util.cc:163: static const size_t kStepSize = 1024 * 1024; On 2013/08/29 17:44:05, Alexei Svitkine wrote: > Nit: No need for static. Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_field_trials_desktop.cc:131: #if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD) On 2013/08/29 17:44:05, Alexei Svitkine wrote: > Is there a reason this should only be activated in a GOOGLE_CHROME_BUILD? > > We don't generally have that restriction on other field trials. Done. https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/23534009/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:717: AddPreReadHistogramTime(name, time); On 2013/08/29 17:44:05, Alexei Svitkine wrote: > On 2013/08/29 15:28:37, Roger McFarlane (Chromium) wrote: > > As this function has now been reduced to nothing, should it be clobbered and > the > > call-sites updated to directly call AddPreREadHistogramTime()? Could the > > call-sites even just be updated to use UMA_HISTOGRAM_*? > > I agree, it should just be replaced with an equivalent UMA_HISTOGRAM_*() macro > call. Done.
Sounds like cpu has outstanding concerns on this patch, please come to some agreement with him.
Thanks. Looking good generally, with a few more comments below. https://codereview.chromium.org/23534009/diff/37001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/app/client_util.cc... chrome/app/client_util.cc:48: static const char kBuildTimeStr[] = __DATE__ " " __TIME__; Nit: static keyword isn't needed for any of these - same thing for the ones in InitPreReadPercentage(). https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:135: env->GetVar(chrome::kPreReadEnvironmentVariable, &group); Can you exit early if this env var isn't set (and not create the trial at all)? This way, if the other bit of code in client_util.cc doesn't run (for whatever reason), we won't be putting folks into any group. Alternatively, make the default group of the experiment "No-Preread" - so that if the client_util.cc code isn't hit for whatever reason - then that indicates the preread codepath wasn't hit and thus there was no preread. https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:143: "BrowserPreReadExperiment", 100, "100%-Default", We generally discourage special characters in the field trial names and group names. I'm not sure if % specifically will cause any issues, but to be safe, I'd use something else, like the word "percent" or "pct". https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:165: trial->AppendGroup("0%", group == "0%" ? 100 : 0); You need to call trial->group() on the experiment for it to be marked as used. Please do it here. https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1509: base::TimeTicks::Now() - browser_open_start); Nit: Move the second argument into a local var.
I need a bit more background here. Once we select to be in a given percentage, how do we persist the value? or we don't? I see how we pass the value from exe to chrome.dll that part seems fine.
@cpu: We derive the experiment group from the user's client/metrics id, which is fixed. We'll always get the same group for a given id, so we don't have to separately persist it. https://codereview.chromium.org/23534009/diff/37001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/app/client_util.cc... chrome/app/client_util.cc:48: static const char kBuildTimeStr[] = __DATE__ " " __TIME__; On 2013/09/03 14:59:16, Alexei Svitkine wrote: > Nit: static keyword isn't needed for any of these - same thing for the ones in > InitPreReadPercentage(). Done. https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:135: env->GetVar(chrome::kPreReadEnvironmentVariable, &group); On 2013/09/03 14:59:16, Alexei Svitkine wrote: > Can you exit early if this env var isn't set (and not create the trial at all)? > > This way, if the other bit of code in client_util.cc doesn't run (for whatever > reason), we won't be putting folks into any group. > > Alternatively, make the default group of the experiment "No-Preread" - so that > if the client_util.cc code isn't hit for whatever reason - then that indicates > the preread codepath wasn't hit and thus there was no preread. The default isn't no-preread, it's full pre-read. the experiment is to find a hypothesized less-than-full pre-read value that's better than 100% https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:143: "BrowserPreReadExperiment", 100, "100%-Default", On 2013/09/03 14:59:16, Alexei Svitkine wrote: > We generally discourage special characters in the field trial names and group > names. > > I'm not sure if % specifically will cause any issues, but to be safe, I'd use > something else, like the word "percent" or "pct". Done. https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_field_trials_desktop.cc:165: trial->AppendGroup("0%", group == "0%" ? 100 : 0); On 2013/09/03 14:59:16, Alexei Svitkine wrote: > You need to call trial->group() on the experiment for it to be marked as used. > Please do it here. Done. https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/37001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1509: base::TimeTicks::Now() - browser_open_start); On 2013/09/03 14:59:16, Alexei Svitkine wrote: > Nit: Move the second argument into a local var. Done.
LGTM Please test this locally to ensure the field trial propagation works as expected. You can do that by visiting chrome://version in your local instance and seeing if your field trial shows up there. (If it's a release build, you'll see hashes there which you can paste into go/finch-hashes to get the names - otherwise you'll see the actual names.) https://codereview.chromium.org/23534009/diff/45001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/45001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1509: base::TimeDelta browser_open_delta = browser_open_end - browser_open_start; Nit: Inline base::TimeTicks::Now(), e.g.: base::TimeDelta startup_time = base::TimeTicks::Now() - browser_open_start;
https://codereview.chromium.org/23534009/diff/45001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23534009/diff/45001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1509: base::TimeDelta browser_open_delta = browser_open_end - browser_open_start; On 2013/09/03 18:48:45, Alexei Svitkine wrote: > Nit: Inline base::TimeTicks::Now(), e.g.: > > base::TimeDelta startup_time = base::TimeTicks::Now() - browser_open_start; Done.
Carlos?
see traffic on the bug.
+rvargas PTAL?
lgtm https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:143: // Persiste the group name to the environment so that it can be used for Persist
lgtm stamp
Just nits. lgtm https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:69: DCHECK(population != NULL); nit: we usually don't dcheck on required arguments being not null. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:104: const uint8 kDefaultPercentage = 100; nit: being just a number, this should be a plain int as opposed to unsigned with specific length. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:118: // We limit experiment populations to 1% of the Stable and 10% of each of nit: indentation https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:130: // increments. This is 21 groups of 5 -- to include the range [100, 105) -- nit: I think it is more clear if you say that value is [0, 100] and multiple of 5. 105 is misleading https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:169: // performs slightly better than 100%. On XP, pre-read is generally a I'm curious as to why it looks like we use preread on XP while knowing that it is a loss.
Thanks. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc File chrome/app/client_util.cc (right): https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:69: DCHECK(population != NULL); On 2013/09/05 22:18:48, rvargas wrote: > nit: we usually don't dcheck on required arguments being not null. Done. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:104: const uint8 kDefaultPercentage = 100; On 2013/09/05 22:18:48, rvargas wrote: > nit: being just a number, this should be a plain int as opposed to unsigned with > specific length. Done. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:118: // We limit experiment populations to 1% of the Stable and 10% of each of On 2013/09/05 22:18:48, rvargas wrote: > nit: indentation Done. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:130: // increments. This is 21 groups of 5 -- to include the range [100, 105) -- On 2013/09/05 22:18:48, rvargas wrote: > nit: I think it is more clear if you say that value is [0, 100] and multiple of > 5. 105 is misleading Done. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:143: // Persiste the group name to the environment so that it can be used for On 2013/09/05 20:21:16, cpu wrote: > Persist Done. https://codereview.chromium.org/23534009/diff/56001/chrome/app/client_util.cc... chrome/app/client_util.cc:169: // performs slightly better than 100%. On XP, pre-read is generally a On 2013/09/05 22:18:48, rvargas wrote: > I'm curious as to why it looks like we use preread on XP while knowing that it > is a loss. My bad, as a pre-experiment, I reverted the value to a constant (100) from (ver > XP ? 25 : 0) and didn't update the comment appropriately. We've seen differing data points between experiments in the lab and histograms in the wild. In the lab, pre-read on XP seems to always be a net loss. Histograms from previous experiments (as well as what happened when we turned pre-read off recently) is that in aggregate we get worse performance with it off. That's a part of the motivation for this experiment being run again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerm@chromium.org/23534009/69001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerm@chromium.org/23534009/101001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerm@chromium.org/23534009/101001
Message was sent while issue was closed.
Change committed as 223070 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
