Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9463026:
Add field trial to extend prerender expiration to 5 minutes. (Closed)
|
Created:
8 years, 10 months ago by dominich Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, tburkard+watch_chromium.org, dominich+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd field trial to extend prerender expiration to 5 minutes.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124777
Patch Set 1 #Patch Set 2 : Refactor as experiment mode #Patch Set 3 : Cleanup #Patch Set 4 : Fix unit tests #
Total comments: 2
Patch Set 5 : Fix sum for stable/beta #
Total comments: 6
Patch Set 6 : Response to cbentzel #Patch Set 7 : Metric system #Patch Set 8 : Fixing messaging for net-internals #
Total comments: 1
Messages
Total messages: 23 (0 generated)
I still need to split the histograms based on this field trial, but I'm concerned about doubling the number of histograms. Do you have any ideas on how to do this?
On Fri, Feb 24, 2012 at 4:40 PM, <dominich@chromium.org> wrote: > Reviewers: tburkard, > > Message: > I still need to split the histograms based on this field trial, but I'm > concerned about doubling the number of histograms. Do you have any ideas > on how > to do this? > Could we just do it for a release and see numbers? Not apples+apples, but.... Another thing is to just augment the FinalStatus histograms, as I think that's really the main thing we care about here. > > Description: > Add field trial to extend prerender expiration to 5 minutes. > > > Please review this at http://codereview.chromium.**org/9463026/<http://codereview.chromium.org/9463... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M chrome/browser/prerender/**prerender_config.h > M chrome/browser/prerender/**prerender_config.cc > M chrome/browser/prerender/**prerender_field_trial.h > M chrome/browser/prerender/**prerender_field_trial.cc > M chrome/browser/prerender/**prerender_manager.cc > M chrome/browser/prerender/**prerender_manager_unittest.cc > M chrome/browser/resources/net_**internals/prerender_view.html > M chrome/browser/resources/net_**internals/prerender_view.js > > > Index: chrome/browser/prerender/**prerender_config.cc > diff --git a/chrome/browser/prerender/**prerender_config.cc > b/chrome/browser/prerender/**prerender_config.cc > index 936c10548cbb5fa52b28b8b21814b6**69f2c396de..** > 41adec635144b0795a7f30ef4b69db**494fdc88f0 100644 > --- a/chrome/browser/prerender/**prerender_config.cc > +++ b/chrome/browser/prerender/**prerender_config.cc > @@ -3,15 +3,21 @@ > // found in the LICENSE file. > > #include "chrome/browser/prerender/**prerender_config.h" > +#include "chrome/browser/prerender/**prerender_field_trial.h" > > namespace prerender { > > Config::Config() : max_bytes(100 * 1024 * 1024), > max_elements(1), > rate_limit_enabled(true), > - max_age(base::TimeDelta::**FromSeconds(30)), > https_allowed(true), > - default_tab_bounds(640, 480) { > + default_tab_bounds(640, 480), > + max_age_short_(base::**TimeDelta::FromSeconds(30)), > + max_age_long_(base::TimeDelta:**:FromSeconds(300)) { > +} > + > +base::TimeDelta Config::max_age() const { > + return IsLongExpiration() ? max_age_long_ : max_age_short_; > } > > } // namespace prerender > Index: chrome/browser/prerender/**prerender_config.h > diff --git a/chrome/browser/prerender/**prerender_config.h > b/chrome/browser/prerender/**prerender_config.h > index 46b83a55300d6ccdf571a0de2ed33d**c353e80cbe..** > 5643e8974de286ec41e8ff06e59b5b**d1e962c212 100644 > --- a/chrome/browser/prerender/**prerender_config.h > +++ b/chrome/browser/prerender/**prerender_config.h > @@ -14,6 +14,9 @@ namespace prerender { > struct Config { > Config(); > > + // Maximum age for a prerendered page until it is removed. > + base::TimeDelta max_age() const; > + > // Maximum memory use for a prerendered page until it is killed. > size_t max_bytes; > > @@ -23,15 +26,16 @@ struct Config { > // Is rate limiting enabled? > bool rate_limit_enabled; > > - // Maximum age for a prerendered page until it is removed. > - base::TimeDelta max_age; > - > // Is https allowed? > bool https_allowed; > > // The default tab bounds used as the prerenderer tab size when the > active tab > // cannot be accessed. > gfx::Rect default_tab_bounds; > + > + private: > + base::TimeDelta max_age_short_; > + base::TimeDelta max_age_long_; > }; > > } // namespace prerender > Index: chrome/browser/prerender/**prerender_field_trial.cc > diff --git a/chrome/browser/prerender/**prerender_field_trial.cc > b/chrome/browser/prerender/**prerender_field_trial.cc > index e5174b6eaa804326158fe81667d973**62dae5c220..** > 28c46cf32bee86ba3757566e997cd7**76f86e81ea 100644 > --- a/chrome/browser/prerender/**prerender_field_trial.cc > +++ b/chrome/browser/prerender/**prerender_field_trial.cc > @@ -22,6 +22,7 @@ namespace prerender { > namespace { > > const char kOmniboxTrialName[] = "PrerenderFromOmnibox"; > +const char kExpirationTrialName[] = "PrerenderLongExpiration"; > > void SetupPrefetchFieldTrial() { > chrome::VersionInfo::Channel channel = chrome::VersionInfo::** > GetChannel(); > @@ -103,6 +104,7 @@ void SetupPrerenderFieldTrial() { > } // end namespace > > void ConfigureOmniboxPrerender(); > +void ConfigureExpiration(); > > void ConfigurePrefetchAndPrerender(**const CommandLine& command_line) { > enum PrerenderOption { > @@ -163,6 +165,7 @@ void ConfigurePrefetchAndPrerender(**const > CommandLine& command_line) { > PrerenderManager::PRERENDER_**MODE_MAX); > > ConfigureOmniboxPrerender(); > + ConfigureExpiration(); > } > > void ConfigureOmniboxPrerender() { > @@ -201,6 +204,22 @@ void ConfigureOmniboxPrerender() { > NetworkActionPredictor::set_**hit_weight(8.0); > } > > +void ConfigureExpiration() { > + // Field trial to extend expiration times. > + const base::FieldTrial::Probability kDivisor = 100; > + > + base::FieldTrial::Probability kLongProbability = 10; > + chrome::VersionInfo::Channel channel = chrome::VersionInfo::** > GetChannel(); > + if (channel == chrome::VersionInfo::CHANNEL_**STABLE || > + channel == chrome::VersionInfo::CHANNEL_**BETA) { > + kLongProbability = 1; > + } > + scoped_refptr<base::**FieldTrial> expiration_trial( > + new base::FieldTrial(**kExpirationTrialName, kDivisor, > + "PrerenderExpirationShort", 2012, 8, 30)); > + expiration_trial->AppendGroup(**"PrerenderExpirationLong", > kLongProbability); > +} > + > bool IsOmniboxEnabled(Profile* profile) { > if (!profile || profile->IsOffTheRecord()) > return false; > @@ -229,4 +248,10 @@ bool IsOmniboxEnabled(Profile* profile) { > group == base::FieldTrial::**kDefaultGroupNumber; > } > > +bool IsLongExpiration() { > + const int group = base::FieldTrialList::**FindValue(** > kExpirationTrialName); > + return group != base::FieldTrial::**kNotFinalized && > + group != base::FieldTrial::**kDefaultGroupNumber; > +} > + > } // namespace prerender > Index: chrome/browser/prerender/**prerender_field_trial.h > diff --git a/chrome/browser/prerender/**prerender_field_trial.h > b/chrome/browser/prerender/**prerender_field_trial.h > index 97b92f0612575a8e2db5dd54294c8a**eab03c21e2..** > 47b942187363781c944624414eb2c3**d26300b87e 100644 > --- a/chrome/browser/prerender/**prerender_field_trial.h > +++ b/chrome/browser/prerender/**prerender_field_trial.h > @@ -21,6 +21,9 @@ void ConfigurePrefetchAndPrerender(**const CommandLine& > command_line); > // prerendering from Omnibox experiment. > bool IsOmniboxEnabled(Profile* profile); > > +// Returns true if the user is in the field trial for longer expiration > times. > +bool IsLongExpiration(); > + > } // namespace prerender > > #endif // CHROME_BROWSER_PRERENDER_**PRERENDER_FIELD_TRIAL_H_ > Index: chrome/browser/prerender/**prerender_manager.cc > diff --git a/chrome/browser/prerender/**prerender_manager.cc > b/chrome/browser/prerender/**prerender_manager.cc > index 8ab48092a24afb4f487dce067f9f4e**06d7d870df..** > 97c351a34c7d07a4d6957112cedf31**afd792ef07 100644 > --- a/chrome/browser/prerender/**prerender_manager.cc > +++ b/chrome/browser/prerender/**prerender_manager.cc > @@ -415,7 +415,7 @@ bool PrerenderManager::**MaybeUsePrerenderedPage(**WebContents* > web_contents, > if (!prerender_contents->load_**start_time().is_null()) { > histograms_->**RecordTimeUntilUsed(**GetCurrentTimeTicks() - > prerender_contents->load_** > start_time(), > - config_.max_age); > + config_.max_age()); > } > > histograms_->**RecordPerSessionCount(++**prerenders_per_session_count_)* > *; > @@ -722,12 +722,13 @@ DictionaryValue* PrerenderManager::GetAsValue() > const { > dict_value->Set("history", prerender_history_->**GetEntriesAsValue()); > dict_value->Set("active", GetActivePrerendersAsValue()); > dict_value->SetBoolean("**enabled", enabled_); > - dict_value->SetBoolean("**omnibox_enabled", > IsOmniboxEnabled(profile_)); > // If prerender is disabled via a flag this method is not even called. > if (IsControlGroup()) > dict_value->SetString("**disabled_reason", "(Disabled for testing)"); > if (IsNoUseGroup()) > dict_value->SetString("**disabled_reason", "(Not using prerendered > pages)"); > + dict_value->SetBoolean("**omnibox_enabled", > IsOmniboxEnabled(profile_)); > + dict_value->SetBoolean("long_**expiration", IsLongExpiration()); > return dict_value; > } > > @@ -987,7 +988,7 @@ void PrerenderManager::**PostCleanupTask() { > bool PrerenderManager::**IsPrerenderElementFresh(const base::Time start) > const { > DCHECK(CalledOnValidThread()); > base::Time now = GetCurrentTime(); > - return (now - start < config_.max_age); > + return (now - start < config_.max_age()); > } > > void PrerenderManager::**DeleteOldEntries() { > Index: chrome/browser/prerender/**prerender_manager_unittest.cc > diff --git a/chrome/browser/prerender/**prerender_manager_unittest.cc > b/chrome/browser/prerender/**prerender_manager_unittest.cc > index 6aac8e5d32a7278c8afa017d657b86**10e455bc77..** > eec9a4b71cf60655a4dcfb716a2fb1**4be2051a75 100644 > --- a/chrome/browser/prerender/**prerender_manager_unittest.cc > +++ b/chrome/browser/prerender/**prerender_manager_unittest.cc > @@ -292,7 +292,7 @@ TEST_F(PrerenderManagerTest, ExpireTest) { > EXPECT_TRUE(prerender_manager(**)->AddSimplePrerender(url)); > EXPECT_EQ(null, prerender_manager()->next_**prerender_contents()); > EXPECT_TRUE(prerender_**contents->has_started()); > - prerender_manager()->**AdvanceTime(prerender_manager(** > )->config().max_age > + prerender_manager()->**AdvanceTime(prerender_manager(** > )->config().max_age() > + base::TimeDelta::FromSeconds(**1)); > ASSERT_EQ(null, prerender_manager()->GetEntry(**url)); > } > Index: chrome/browser/resources/net_**internals/prerender_view.html > diff --git a/chrome/browser/resources/**net_internals/prerender_view.**html > b/chrome/browser/resources/**net_internals/prerender_view.**html > index 3296ed5a4d68cb8f83d7082a319f2d**495688443e..** > ea0814dde24d3544cbc29e1b48f225**56c935a642 100644 > --- a/chrome/browser/resources/**net_internals/prerender_view.**html > +++ b/chrome/browser/resources/**net_internals/prerender_view.**html > @@ -4,6 +4,8 @@ > <li>Prerender Enabled: <span id=prerender-view-enabled-** > span></span></li> > <li>Prerender Omnibox Enabled: > <span id=prerender-view-omnibox-**enabled-span></span></li> > + <li>Long expiration: > + <span id=prerender-view-long-**expiration-span></span></li> > </ul> > <p> > Active Prerender Pages > Index: chrome/browser/resources/net_**internals/prerender_view.js > diff --git a/chrome/browser/resources/**net_internals/prerender_view.**js > b/chrome/browser/resources/**net_internals/prerender_view.**js > index be64c439f39c1755cdc1cc495eff00**05d226eb88..** > 372443eb503becabceb60d95b38404**6c8241d22c 100644 > --- a/chrome/browser/resources/**net_internals/prerender_view.**js > +++ b/chrome/browser/resources/**net_internals/prerender_view.**js > @@ -24,6 +24,8 @@ var PrerenderView = (function() { > this.prerenderEnabledSpan_ = $(PrerenderView.ENABLED_SPAN_**ID); > this.**prerenderOmniboxEnabledSpan_ = > $(PrerenderView.OMNIBOX_**ENABLED_SPAN_ID); > + this.**prerenderLongExpiratonSpan_ = > + $(PrerenderView.LONG_**EXPIRATION_SPAN_ID); > this.prerenderHistoryDiv_ = $(PrerenderView.HISTORY_DIV_**ID); > this.prerenderActiveDiv_ = $(PrerenderView.ACTIVE_DIV_ID)**; > } > @@ -35,6 +37,7 @@ var PrerenderView = (function() { > PrerenderView.MAIN_BOX_ID = 'prerender-view-tab-content'; > PrerenderView.ENABLED_SPAN_ID = 'prerender-view-enabled-span'; > PrerenderView.OMNIBOX_ENABLED_**SPAN_ID = 'prerender-view-omnibox-** > enabled-span'; > + PrerenderView.LONG_EXPIRATION_**SPAN_ID = 'prerender-view-long-** > expiration-span'; > PrerenderView.HISTORY_DIV_ID = 'prerender-view-history-div'; > PrerenderView.ACTIVE_DIV_ID = 'prerender-view-active-div'; > > @@ -51,6 +54,7 @@ var PrerenderView = (function() { > onPrerenderInfoChanged: function(prerenderInfo) { > this.prerenderEnabledSpan_.**textContent = ''; > this.**prerenderOmniboxEnabledSpan_.**textContent = ''; > + this.**prerenderLongExpiratonSpan_.**textContnt = ''; > this.prerenderHistoryDiv_.**innerHTML = ''; > this.prerenderActiveDiv_.**innerHTML = ''; > > @@ -68,6 +72,11 @@ var PrerenderView = (function() { > prerenderInfo.omnibox_enabled.**toString(); > } > > + if (prerenderInfo && ('long_expiration' in prerenderInfo)) { > + this.**prerenderLongExpiratonSpan_.**textContent = > + prerenderInfo.long_expiration.**toString(); > + } > + > if (!isValidPrerenderInfo(**prerenderInfo)) > return false; > > > >
On 2012/02/24 22:18:19, cbentzel wrote: > On Fri, Feb 24, 2012 at 4:40 PM, <mailto:dominich@chromium.org> wrote: > > > Reviewers: tburkard, > > > > Message: > > I still need to split the histograms based on this field trial, but I'm > > concerned about doubling the number of histograms. Do you have any ideas > > on how > > to do this? > > > > Could we just do it for a release and see numbers? Not apples+apples, > but.... > > Another thing is to just augment the FinalStatus histograms, as I think > that's really the main thing we care about here. After discussing with Timo, I'm going to try adding it as a new group alongside the experiment/control.
This is ready for review
http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_field_trial.cc (right): http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_field_trial.cc:60: exp1_probability = 490; These no longer sum to 1200
http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_field_trial.cc (right): http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_field_trial.cc:60: exp1_probability = 490; On 2012/02/27 19:23:20, cbentzel wrote: > These no longer sum to 1200 Nice catch - that would have been a release check fail in beta. Maybe we should extract the stable/beta constants from the dev constants and do two checks to ensure the sums are correct.
On 2012/02/27 19:29:53, dominich wrote: > http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_field_trial.cc (right): > > http://codereview.chromium.org/9463026/diff/3005/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_field_trial.cc:60: exp1_probability = 490; > On 2012/02/27 19:23:20, cbentzel wrote: > > These no longer sum to 1200 > > Nice catch - that would have been a release check fail in beta. > > Maybe we should extract the stable/beta constants from the dev constants and do > two checks to ensure the sums are correct. I'd really like to keep divisor to 1000 so it's easier to keep track of things. I guess this means I prefer the metric system.
Are you doing the histogram changes in another CL? http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_field_trial.cc (right): http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_field_trial.cc:48: base::FieldTrial::Probability exp1_long_ttl_probability = 100; Should this be named "5 minute TTL" or something explicit like that? If we wanted to do another experiment with 10 minutes don't want to name it "longer" or reuse "long" . http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:732: dict_value->SetString("disabled_reason", "(Long TTL)"); Perhaps "disabled_reason" should change names? http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:992: max_age *= 10; Should this be constant rather than ratio?
On 2012/02/28 00:53:40, cbentzel wrote: > Are you doing the histogram changes in another CL? Ahh - with PrerenderManagerMode this isn't needed. Clever. > > http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_field_trial.cc (right): > > http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_field_trial.cc:48: > base::FieldTrial::Probability exp1_long_ttl_probability = 100; > Should this be named "5 minute TTL" or something explicit like that? If we > wanted to do another experiment with 10 minutes don't want to name it "longer" > or reuse "long" . > > http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... > File chrome/browser/prerender/prerender_manager.cc (right): > > http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.cc:732: > dict_value->SetString("disabled_reason", "(Long TTL)"); > Perhaps "disabled_reason" should change names? > > http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... > chrome/browser/prerender/prerender_manager.cc:992: max_age *= 10; > Should this be constant rather than ratio?
I also like the metric system, but in this case the numbers make more sense in the duodecimal system. http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_field_trial.cc (right): http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_field_trial.cc:48: base::FieldTrial::Probability exp1_long_ttl_probability = 100; On 2012/02/28 00:53:41, cbentzel wrote: > Should this be named "5 minute TTL" or something explicit like that? If we > wanted to do another experiment with 10 minutes don't want to name it "longer" > or reuse "long" . Done. http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:732: dict_value->SetString("disabled_reason", "(Long TTL)"); On 2012/02/28 00:53:41, cbentzel wrote: > Perhaps "disabled_reason" should change names? Done. http://codereview.chromium.org/9463026/diff/9002/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:992: max_age *= 10; On 2012/02/28 00:53:41, cbentzel wrote: > Should this be constant rather than ratio? Done.
On 2012/02/28 17:51:31, dominich wrote: > I also like the metric system, but in this case the numbers make more sense in > the duodecimal system. I really strongly disagree. It's faster when writing to use your divisor, but much slower when reading and trying to understand what percentage of users are in a particular bucket. I'll still give an LGTM, but I hope this changes to a 1000 divisor.
tburkard: ping
Thanks for making the change. LGTM On Thu, Mar 1, 2012 at 10:53 AM, <dominich@chromium.org> wrote: > tburkard: ping > > http://codereview.chromium.**org/9463026/<http://codereview.chromium.org/9463... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominich@chromium.org/9463026/22001
Presubmit check for 9463026-22001 failed and returned exit status 1. /mnt/data/b/commit-queue/workdir/chromium/third_party/closure_linter/closure_linter/javascriptlintrules.py:28: DeprecationWarning: the sets module is deprecated from sets import Set Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/resources/net_internals/prerender_view.js Presubmit checks took 1.6s to calculate.
mmenke: net-internals
On 2012/03/02 18:25:49, dominich wrote: > mmenke: net-internals LGTM, though perhaps we should make the strings a little clearer. I've had to explain what "true (Disabled for testing)" means more than once.
PTAL: Updated enabled_note text.
LGTM, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominich@chromium.org/9463026/29002
Change committed as 124777
:-( http://codereview.chromium.org/9463026/diff/29002/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9463026/diff/29002/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.h:72: PRERENDER_MODE_EXPERIMENT_5MIN_TTL_GROUP, This should never have been committed! See the comment at the top of this enum. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
