Make three simultanious prerenders the default maximum in Canary and Dev only.

Make three simultanious prerenders the default maximum in Canary and Dev only.

Very basic support for three simultanious prerenders, as a basis for continued updates on the API.

R=dominich@chromium.org
BUG=None


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

[gavinp, 2012-07-19 10:54:39]

Dominic,

Here's a nice incremental start to upping our prerender count. I suspect we want
a few more histograms (perhaps about system memory?), and we likely want to
guard against too-simultaneous launch.

The existing unit tests and browser tests ended up testing this a good amount;
there's lots of times in testing with multiple prerenders in flight, and we do
end up checking that they expire correctly.

WDYT?

[gavinp, 2012-07-19 11:06:54]

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:236: const content::Referrer&
referrer,
... (ooops)

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:1552:
EXPECT_TRUE(url_b_is_active_prerender && url_c_is_active_prerender);
Of course, now this test depends on concurrency >= 2.

[dominich, 2012-07-19 16:04:38]

I like how focused this CL is, and it's mostly there. Just one big change, and a
few nits.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:1529: // be evicted, and the
second should stick around until we exit.
update comment.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:1552:
EXPECT_TRUE(url_b_is_active_prerender && url_c_is_active_prerender);
On 2012/07/19 11:06:54, gavinp wrote:
> Of course, now this test depends on concurrency >= 2.

Which is accurate. If you want to be very careful, add an
ASSERT_GT(prerender_manager_concurrent_prerender(), 1);

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:8: #include <string>
wat?

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const int
kDefaultMaxConcurrency = 2;
this doesn't need to be in the header.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:23: unsigned int max_concurrency;
unsigned int, but the default is int? Sure, if someone sets the default to
negative we'll probably catch that in review, but why not make it consistent?

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:51:
base::FieldTrial::Probability exp1_probability = 150;
I would very much like to remove half of these now that we're confident that
variance within each group is within acceptable bounds.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:55:
base::FieldTrial::Probability one_prerender1_probability = 66;
We should flip this around. have exp[12] be single prerender as it is now, and
add an experiment for multiple prerenders.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:154:
Config::kDefaultMaxConcurrency + 1);
this will be a measure of how often we prerender 1 vs 2 (vs ?) prererenders
concurrently? Does this number then need to be modulated by experiment, ie,
should we only record this for the experiment where maximum concurrency is > 1?
Or, even better, add a separate histogram for each maximum concurrency:

StringPrintf("Prerender.ConcurrentPrerenderCount%d",
PrerenderManager::config().concurrency)

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.h:80: void
RecordPrerenderStarted(Origin origin, size_t prerender_count) const;
the max in config is an unsigned int (32-bit) but the count here is a size_t
(possibly 64-bit).

I think the max in config should be a size_t. It's the correct type for what
this code is doing and will enable consistency.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
histograms_->RecordPrerenderStarted(origin, active_prerender_list_.size());
changing this is bad - we'll have a mismatch between started and evicted which
Omnibox stats rely on.

Instead, record started as before and maybe separate the count tracking and do
that here.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:81:
PRERENDER_MODE_EXPERIMENT_ONE_PRERENDER_GROUP,
MULTIPLE_PRERENDER_GROUP

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_unittest.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:395: concurrencies_to_test.at(0)
= prerender_manager()->config().max_concurrency;
concurrencies_to_test[0] = ...

or concurrencies_to_test.push_back(...)

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:402: for
(std::vector<int>::size_type i = 0;
iterator loop would be better here.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:416:
std::vector<PrerenderContents*> prerender_contentses;
what's he got in his prerender contentses, my precious?

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:430:
ASSERT_EQ(prerender_contentses.at(j),
prerender_contentses[j]

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:431:
prerender_manager()->FindAndUseEntry(urls.at(j)));
urls[j]

[gavinp, 2012-07-23 18:35:21]

How's this looking?

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:1529: // be evicted, and the
second should stick around until we exit.
On 2012/07/19 16:04:38, dominich wrote:
> update comment.

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_browsertest.cc:1552:
EXPECT_TRUE(url_b_is_active_prerender && url_c_is_active_prerender);
On 2012/07/19 16:04:38, dominich wrote:
> On 2012/07/19 11:06:54, gavinp wrote:
> > Of course, now this test depends on concurrency >= 2.
> 
> Which is accurate. If you want to be very careful, add an
> ASSERT_GT(prerender_manager_concurrent_prerender(), 1);

Since in another review comment, you asked me to make the default 1, instead I
made this test default to 2. So that acts similarly to an assertion, the setting
of concurrency.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:8: #include <string>
On 2012/07/19 16:04:38, dominich wrote:
> wat?

This is a cleanup to make this file pass lint. See user_agent_override below.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const int
kDefaultMaxConcurrency = 2;
On 2012/07/19 16:04:38, dominich wrote:
> this doesn't need to be in the header.

It's used in prerender_histograms.cc though. What do you suggest for sizing the
histogram? I renamed it to kMaximumMaxConcurrency, since the default is now 1.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:23: unsigned int max_concurrency;
On 2012/07/19 16:04:38, dominich wrote:
> unsigned int, but the default is int? Sure, if someone sets the default to
> negative we'll probably catch that in review, but why not make it consistent?

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:51:
base::FieldTrial::Probability exp1_probability = 150;
On 2012/07/19 16:04:38, dominich wrote:
> I would very much like to remove half of these now that we're confident that
> variance within each group is within acceptable bounds.

I like that plan, but I figured a separate CL for cleaning this up makes more
sense. See http://codereview.chromium.org/10802062/ , which my upload rebases
on.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:55:
base::FieldTrial::Probability one_prerender1_probability = 66;
On 2012/07/19 16:04:38, dominich wrote:
> We should flip this around. have exp[12] be single prerender as it is now, and
> add an experiment for multiple prerenders.
> 

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:154:
Config::kDefaultMaxConcurrency + 1);
On 2012/07/19 16:04:38, dominich wrote:
> this will be a measure of how often we prerender 1 vs 2 (vs ?) prererenders
> concurrently? Does this number then need to be modulated by experiment, ie,
> should we only record this for the experiment where maximum concurrency is >
1?
> Or, even better, add a separate histogram for each maximum concurrency:
> 
> StringPrintf("Prerender.ConcurrentPrerenderCount%d",
> PrerenderManager::config().concurrency)

Good idea: I now only emit this if the max concurrency > 1. I also added the
integer to the name.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.h:80: void
RecordPrerenderStarted(Origin origin, size_t prerender_count) const;
On 2012/07/19 16:04:38, dominich wrote:
> the max in config is an unsigned int (32-bit) but the count here is a size_t
> (possibly 64-bit).
> 
> I think the max in config should be a size_t. It's the correct type for what
> this code is doing and will enable consistency.

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
histograms_->RecordPrerenderStarted(origin, active_prerender_list_.size());
On 2012/07/19 16:04:38, dominich wrote:
> changing this is bad - we'll have a mismatch between started and evicted which
> Omnibox stats rely on.
> 
> Instead, record started as before and maybe separate the count tracking and do
> that here.

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:81:
PRERENDER_MODE_EXPERIMENT_ONE_PRERENDER_GROUP,
On 2012/07/19 16:04:38, dominich wrote:
> MULTIPLE_PRERENDER_GROUP

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_unittest.cc (right):

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:395: concurrencies_to_test.at(0)
= prerender_manager()->config().max_concurrency;
On 2012/07/19 16:04:38, dominich wrote:
> concurrencies_to_test[0] = ...
> 
> or concurrencies_to_test.push_back(...)

Done. Although I really like vector<>::at() for its safety vs [].

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:402: for
(std::vector<int>::size_type i = 0;
On 2012/07/19 16:04:38, dominich wrote:
> iterator loop would be better here.

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:416:
std::vector<PrerenderContents*> prerender_contentses;
On 2012/07/19 16:04:38, dominich wrote:
> what's he got in his prerender contentses, my precious?

I could have stayed with prerender_contents, but this name conveyed plurality.
But it does sound silly.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:430:
ASSERT_EQ(prerender_contentses.at(j),
On 2012/07/19 16:04:38, dominich wrote:
> prerender_contentses[j]

Done.

http://codereview.chromium.org/10802024/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:431:
prerender_manager()->FindAndUseEntry(urls.at(j)));
On 2012/07/19 16:04:38, dominich wrote:
> urls[j]

Done.

[dominich, 2012-07-23 18:45:56]

Looking good.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const size_t
kMaximumMaxConcurrency = 3;
This isn't referenced. PrerenderManager sets '3' explicitly.

I'm ok with that, but you should remove this.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
why is this important? if we want to experiment with 10, we shouldn't have to
update that in two places.

[mmenke, 2012-07-23 18:48:49]

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
Drive by nit:  DCHECK_GE

[gavinp, 2012-07-23 18:51:55]

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const size_t
kMaximumMaxConcurrency = 3;
On 2012/07/23 18:45:56, dominich wrote:
> This isn't referenced. PrerenderManager sets '3' explicitly.
> 
> I'm ok with that, but you should remove this.

See
http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
, line 167. The histogram has to be sized right, or it's wasting RAM.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
On 2012/07/23 18:45:56, dominich wrote:
> why is this important? if we want to experiment with 10, we shouldn't have to
> update that in two places.

This test guards the histograms sizing; the alternative was getters and setters
on the config object.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
On 2012/07/23 18:48:49, Matt Menke wrote:
> Drive by nit:  DCHECK_GE

DCHECK_GE won't work, as it takes a pointer to its arguments, and that is a
static const int, which doesn't have data segment extent if it isn't
dereferenced. So using DCHECK_GE will cause a linker error. Do you think it's
worth switching to DCHECK_GE if the cost is adding the const to a data segment?

[dominich, 2012-07-23 18:57:14]

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const size_t
kMaximumMaxConcurrency = 3;
On 2012/07/23 18:51:55, gavinp wrote:
> On 2012/07/23 18:45:56, dominich wrote:
> > This isn't referenced. PrerenderManager sets '3' explicitly.
> > 
> > I'm ok with that, but you should remove this.
> 
> See
>
http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
> , line 167. The histogram has to be sized right, or it's wasting RAM.
> 

I see. maybe instead of an enum you should be using UMA_HISTOGRAM_COUNTS ?

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
On 2012/07/23 18:51:55, gavinp wrote:
> On 2012/07/23 18:48:49, Matt Menke wrote:
> > Drive by nit:  DCHECK_GE
> 
> DCHECK_GE won't work, as it takes a pointer to its arguments, and that is a
> static const int, which doesn't have data segment extent if it isn't
> dereferenced. So using DCHECK_GE will cause a linker error. Do you think it's
> worth switching to DCHECK_GE if the cost is adding the const to a data
segment?

in logging.h it uses const T& or int - I don't see any pointers being taken.

[gavinp, 2012-07-23 19:06:10]

OK, OK, I cleaned up the insanely confusing constant.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_config.h (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_config.h:22: static const size_t
kMaximumMaxConcurrency = 3;
On 2012/07/23 18:57:14, dominich wrote:
> On 2012/07/23 18:51:55, gavinp wrote:
> > On 2012/07/23 18:45:56, dominich wrote:
> > > This isn't referenced. PrerenderManager sets '3' explicitly.
> > > 
> > > I'm ok with that, but you should remove this.
> > 
> > See
> >
>
http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
> > , line 167. The histogram has to be sized right, or it's wasting RAM.
> > 
> 
> I see. maybe instead of an enum you should be using UMA_HISTOGRAM_COUNTS ?

Well, that's just giving up and wasting a lot more memory. UMA_HISTOGRAM_COUNTS
always uses 50 buckets. I have actually been changing our uses of COUNTS away
from that, just as a quick CL to save memory. I have added a constant here, and
now I'm DCHECKing on that. That should be good enough to have us resize the
histogram when we need to.

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/10802024/diff/15001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:955:
DCHECK(Config::kMaximumMaxConcurrency >= config_.max_concurrency);
On 2012/07/23 18:57:14, dominich wrote:
> On 2012/07/23 18:51:55, gavinp wrote:
> > On 2012/07/23 18:48:49, Matt Menke wrote:
> > > Drive by nit:  DCHECK_GE
> > 
> > DCHECK_GE won't work, as it takes a pointer to its arguments, and that is a
> > static const int, which doesn't have data segment extent if it isn't
> > dereferenced. So using DCHECK_GE will cause a linker error. Do you think
it's
> > worth switching to DCHECK_GE if the cost is adding the const to a data
> segment?
> 
> in logging.h it uses const T& or int - I don't see any pointers being taken.

Sorry, takes a reference.

[dominich, 2012-07-23 19:10:17]

LGTM

[gavinp, 2012-07-23 19:26:50]

On 2012/07/23 19:10:17, dominich wrote:
> LGTM

ty. I caught a problem in my prerender_manager.h.

Since this change depends on my field trial changes, I'll CQ+ it as soon as
those have safely landed.

[mmenke, 2012-07-26 16:29:48]

Sorry, forgot I was a reviewer on this one.  LGTM.

http://codereview.chromium.org/10802024/diff/7008/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/10802024/diff/7008/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:172: 
nit:  Remove extra blank line.

[mmenke, 2012-07-26 16:33:07]

On 2012/07/26 16:29:48, Matt Menke wrote:
> Sorry, forgot I was a reviewer on this one.  LGTM.

Ahh...I wasn't...the drive by nit, right.  Forgot about that.

[commit-bot: I haz the power, 2012-07-27 15:55:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/10802024/23003

[commit-bot: I haz the power, 2012-07-27 16:56:47]

Change committed as 148761