Limit prerender by only allowing one every X ms (X currently set to 500).

Limit prerender by only allowing one every X ms (X currently set to 500).

BUG=
TEST=Run a search that returns a prerender link ('cnn') and without navigating immediately run another ('nytimes'). Observe the second search does not cause a prerender if performed within the rate limit.

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

[dominich, 2011-03-11 22:32:22]

Only allow subsequent prerenders after X ms. I set the time between prerenders
to 500 ms after a bit of tweaking and added an interface to disable it so unit
tests don't have to be artificially delayed.

[tburkard, 2011-03-11 22:57:40]

LGTM

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:23: #include
"content/browser/renderer_host/resource_dispatcher_host.h"
why does this need to be added?

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:170: static const int
kMinTimeBetweenPrerendersMs = 500;
I think we should set this to 1s.

[dominich, 2011-03-11 23:14:57]

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:23: #include
"content/browser/renderer_host/resource_dispatcher_host.h"
On 2011/03/11 22:57:40, tburkard wrote:
> why does this need to be added?

it doesn't - i have no idea why that's there.

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:170: static const int
kMinTimeBetweenPrerendersMs = 500;
On 2011/03/11 22:57:40, tburkard wrote:
> I think we should set this to 1s.

I originally had it at 1s but felt it was too long. I did some vaguely empirical
testing by running searches and could easily get two searches in under 1s.

As this is only meant to catch a situation where we're being flooded, I felt
that 500 was a good balance. Do you have reason to think otherwise?

[tburkard, 2011-03-11 23:19:21]

Ok, let's do 500ms, but how about you add the following: a histogram for
time between two consecutive prerenders!

And while we're at it, could you add # of prerenders performed per session
as a histogram.  The way to do this is to simply have a static counter,
initialized to 0, and every time you perform a prerender, you incremen the
counter and emit the current counter value to the histogram.  That way, you
will get a cumulated histogram, i.e. a value of X will mean "how many
sessions generated X or more prerenders".

Timo

On Fri, Mar 11, 2011 at 3:14 PM, <dominich@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_contents.cc (right):
>
>
>
http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_contents.cc:23: #include
> "content/browser/renderer_host/resource_dispatcher_host.h"
> On 2011/03/11 22:57:40, tburkard wrote:
>
>> why does this need to be added?
>>
>
> it doesn't - i have no idea why that's there.
>
>
>
>
http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_manager.h (right):
>
>
>
http://codereview.chromium.org/6677005/diff/2001/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_manager.h:170: static const int
> kMinTimeBetweenPrerendersMs = 500;
> On 2011/03/11 22:57:40, tburkard wrote:
>
>> I think we should set this to 1s.
>>
>
> I originally had it at 1s but felt it was too long. I did some vaguely
> empirical testing by running searches and could easily get two searches
> in under 1s.
>
> As this is only meant to catch a situation where we're being flooded, I
> felt that 500 was a good balance. Do you have reason to think otherwise?
>
>
> http://codereview.chromium.org/6677005/
>

[dominich, 2011-03-11 23:43:22]

On 2011/03/11 23:19:21, tburkard wrote:
> Ok, let's do 500ms, but how about you add the following: a histogram for
> time between two consecutive prerenders!

Done.

> 
> And while we're at it, could you add # of prerenders performed per session
> as a histogram.  The way to do this is to simply have a static counter,
> initialized to 0, and every time you perform a prerender, you incremen the
> counter and emit the current counter value to the histogram.  That way, you
> will get a cumulated histogram, i.e. a value of X will mean "how many
> sessions generated X or more prerenders".

I'll add this as a separate CL.

[tburkard, 2011-03-11 23:49:07]

LGTM

http://codereview.chromium.org/6677005/diff/3006/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/3006/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:306:
UMA_HISTOGRAM_ENUMERATION("Prerender.TimeBetweenRequests",
To clarify the name, maybe call it TimeBetweenPrerenderRequests (as opposed to
user-initiated visibile requests).

[cbentzel, 2011-03-14 14:40:32]

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:100: static void
set_rate_limit_enabled(bool enable) {
Why are these done statically? Why not per-PrerenderManager?

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager_unittest.cc:105:
PrerenderManager::set_rate_limit_enabled(false);
This should restore the value in teardown.

[cbentzel, 2011-03-14 14:43:56]

Also - is this behavior what we actually want?

Given the test described (a second prerender link comes about due to user
action), it seems like we actually don't want the rate limit.

Perhaps it's just a matter of tweaking - for example, limiting to 5ms might not
trigger on this case, but would help with some sort of automated abuse.

[dominich, 2011-03-14 17:08:58]

On 2011/03/14 14:43:56, cbentzel wrote:
> Also - is this behavior what we actually want?
> 
> Given the test described (a second prerender link comes about due to user
> action), it seems like we actually don't want the rate limit.
> 
> Perhaps it's just a matter of tweaking - for example, limiting to 5ms might
not
> trigger on this case, but would help with some sort of automated abuse.

The other CL I have http://codereview.chromium.org/6625066 fixes the one issue
that we know of where we can end up prerendering much more than we expect. This
is a catch-all to trap any other times that we might otherwise add to server
load or bandwidth usage.

I agree that the time limit can be tweaked and I'm happy to change it to
shorter. 500 ms seemed like a good first option (after some experimentation) but
the histogram, along with the final status, should tell us if it's too long.

[tburkard, 2011-03-14 17:17:40]

I'd prefer keeping it at 500ms.  The genesis of this was some histograms
where prerenders would happen basically continuously.  I think we shoudl
rather err on the side of not prerendering than potentially using up a lot
of bandwidth and cpu.  I think we should go with 500ms first and then review
the histogram data and re-evaluate.

Timo

On Mon, Mar 14, 2011 at 10:08 AM, <dominich@chromium.org> wrote:

> On 2011/03/14 14:43:56, cbentzel wrote:
>
>> Also - is this behavior what we actually want?
>>
>
>  Given the test described (a second prerender link comes about due to user
>> action), it seems like we actually don't want the rate limit.
>>
>
>  Perhaps it's just a matter of tweaking - for example, limiting to 5ms
>> might
>>
> not
>
>> trigger on this case, but would help with some sort of automated abuse.
>>
>
> The other CL I have http://codereview.chromium.org/6625066 fixes the one
> issue
> that we know of where we can end up prerendering much more than we expect.
> This
> is a catch-all to trap any other times that we might otherwise add to
> server
> load or bandwidth usage.
>
> I agree that the time limit can be tweaked and I'm happy to change it to
> shorter. 500 ms seemed like a good first option (after some
> experimentation) but
> the histogram, along with the final status, should tell us if it's too
> long.
>
>
> http://codereview.chromium.org/6677005/
>

[cbentzel, 2011-03-14 17:27:10]

OK. I'd still like to change these to non-static, as that does not look like
it's needed.

On Mon, Mar 14, 2011 at 1:17 PM, Timo Burkard <tburkard@chromium.org> wrote:

> I'd prefer keeping it at 500ms.  The genesis of this was some histograms
> where prerenders would happen basically continuously.  I think we shoudl
> rather err on the side of not prerendering than potentially using up a lot
> of bandwidth and cpu.  I think we should go with 500ms first and then review
> the histogram data and re-evaluate.
>
> Timo
>
>
> On Mon, Mar 14, 2011 at 10:08 AM, <dominich@chromium.org> wrote:
>
>> On 2011/03/14 14:43:56, cbentzel wrote:
>>
>>> Also - is this behavior what we actually want?
>>>
>>
>>  Given the test described (a second prerender link comes about due to user
>>> action), it seems like we actually don't want the rate limit.
>>>
>>
>>  Perhaps it's just a matter of tweaking - for example, limiting to 5ms
>>> might
>>>
>> not
>>
>>> trigger on this case, but would help with some sort of automated abuse.
>>>
>>
>> The other CL I have http://codereview.chromium.org/6625066 fixes the one
>> issue
>> that we know of where we can end up prerendering much more than we expect.
>> This
>> is a catch-all to trap any other times that we might otherwise add to
>> server
>> load or bandwidth usage.
>>
>> I agree that the time limit can be tweaked and I'm happy to change it to
>> shorter. 500 ms seemed like a good first option (after some
>> experimentation) but
>> the histogram, along with the final status, should tell us if it's too
>> long.
>>
>>
>> http://codereview.chromium.org/6677005/
>>
>
>

[dominich, 2011-03-14 18:10:10]

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:100: static void
set_rate_limit_enabled(bool enable) {
On 2011/03/14 14:40:32, cbentzel wrote:
> Why are these done statically? Why not per-PrerenderManager? 

Done.

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6677005/diff/8003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager_unittest.cc:105:
PrerenderManager::set_rate_limit_enabled(false);
On 2011/03/14 14:40:32, cbentzel wrote:
> This should restore the value in teardown.

When it's no longer static, this is unnecessary.

[cbentzel, 2011-03-14 18:18:15]

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:63:
last_prerender_start_time_(base::TimeTicks::Now()) {
This should be Now - kMinTimeBetweenPrerendersMs so it can work at startup

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:293: // static
Nit: this comment is no longer true.

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:298: if
(last_prerender_start_time_.is_null())
I don't think you need to test this case given how it is initialized in
constructor.

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:302:
UMA_HISTOGRAM_TIMES("Prerender.TimeBetweenPrerenderRequests",
Should this be captured at the start of the function?

[dominich, 2011-03-14 20:03:21]

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:63:
last_prerender_start_time_(base::TimeTicks::Now()) {
On 2011/03/14 18:18:16, cbentzel wrote:
> This should be Now - kMinTimeBetweenPrerendersMs so it can work at startup

Done.

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:293: // static
On 2011/03/14 18:18:16, cbentzel wrote:
> Nit: this comment is no longer true.

Done.

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:298: if
(last_prerender_start_time_.is_null())
On 2011/03/14 18:18:16, cbentzel wrote:
> I don't think you need to test this case given how it is initialized in
> constructor.

Done.

http://codereview.chromium.org/6677005/diff/10002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:302:
UMA_HISTOGRAM_TIMES("Prerender.TimeBetweenPrerenderRequests",
On 2011/03/14 18:18:16, cbentzel wrote:
> Should this be captured at the start of the function? 

It was here as the last_prerender_start_time_ could have been null before.
Moved.

[cbentzel, 2011-03-15 18:51:02]

LGTM

Update issue comment to mention that X is 500 rather than 1000

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:64:
base::TimeDelta::FromMilliseconds(kMinTimeBetweenPrerendersMs)) {
Nit: should indentation be only four from previous line? Not sure in this case.

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:140: bool
DoesRateLimitAllowPrerender();
const

[dominich, 2011-03-15 21:05:37]

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:64:
base::TimeDelta::FromMilliseconds(kMinTimeBetweenPrerendersMs)) {
On 2011/03/15 18:51:03, cbentzel wrote:
> Nit: should indentation be only four from previous line? Not sure in this
case.

I'm not sure either, and I couldn't find anything clear for this case. Down to 4
anyway.

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6677005/diff/11005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:140: bool
DoesRateLimitAllowPrerender();
On 2011/03/15 18:51:03, cbentzel wrote:
> const

Done.

[cbentzel, 2011-03-16 01:06:01]

Sorry for missing this in previous rounds - a unit test should be added to
prerender_manager_unittest.cc, this comment suggests how to do it in a non-flaky
manner.

http://codereview.chromium.org/6677005/diff/14001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/14001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:297: base::TimeTicks::Now() -
last_prerender_start_time_;
This should use GetCurrentTime instead of base::TimeTicks::Now - that way you
can reliably test this in a unit test.

The same holds for the other callers of base::TimeTicks::Now

[dominich, 2011-03-16 19:58:39]

http://codereview.chromium.org/6677005/diff/14001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6677005/diff/14001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:297: base::TimeTicks::Now() -
last_prerender_start_time_;
On 2011/03/16 01:06:01, cbentzel wrote:
> This should use GetCurrentTime instead of base::TimeTicks::Now - that way you
> can reliably test this in a unit test.
> 
> The same holds for the other callers of base::TimeTicks::Now

I added GetCurrentTimeTicks() and used that everywhere instead. I didn't change
the recorded window stuff as that is static and is not unit tested (currently?)

[cbentzel, 2011-03-18 17:31:19]

LGTM

It looks like we should use TimeTicks for the freshness case as well [instead of
Time]. I will change that in a separate CL.

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:308:
EXPECT_FALSE(prerender_manager_->AddSimplePreload(url1));
You should also test that the rate limit does not kick in when outside the
window.

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:312: 
Nit: extra newline

[dominich, 2011-03-18 19:11:46]

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:308:
EXPECT_FALSE(prerender_manager_->AddSimplePreload(url1));
On 2011/03/18 17:31:19, cbentzel wrote:
> You should also test that the rate limit does not kick in when outside the
> window. 

Done.

http://codereview.chromium.org/6677005/diff/18001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:312: 
On 2011/03/18 17:31:19, cbentzel wrote:
> Nit: extra newline

Done.

[cbentzel, 2011-03-18 19:13:18]

LGTM