Implement prefetching in chrome

Implement prefetching in chrome

With this CL (see also issue 2910009), chrome will support basic
prefetching.  You can optionally deactivate prefetching with the
command line argument --disable-prefetch.

A new RequestPriority was created as well, IDLE, which is lower 
than LOWEST.  Unfortunately, SPDY has only two bits for priority, so
as a temporary measure (pending SPDY v3 which will have three), we
have a mapping in SPDY that folds net::LOWEST and net::IDLE together.

BUG=13505
TEST=http://gemal.dk/browserspy/prefetch.php

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

[gavinp, 2010-07-24 01:30:25]

Here's my second pass at this patch; Chris I've incorporated some of your
comments from the last issue.  Most pointedly, though, I've not done anything
with your suggestion to move calls to stop passing net::RequestPriority around
in SPDY code so much, and get to spdy_priority at an higher level.

My reasoning there was that hopefully the difference between the SPDY priority
scheme and the Chrome one will become trivial again soon, so the TODO captures
this.

[gavinp, 2010-07-24 01:40:48]

(I've got a functional test in the works as well, but it's hit a few snags so I
didn't want to delay this feature.  As well, I will be adding A/B trials and
more histograms shortly to measure the impact of prefetching)

[tonyg, 2010-07-26 16:15:37]

Looking great. All my comments are minor/trivial nits.

I'd also be interested in seeing some unittest updates with this patch.

Once it is ready, mbelshe should probably take a look too.

http://codereview.chromium.org/3050016/diff/1/2
File build/features_override.gypi (right):

http://codereview.chromium.org/3050016/diff/1/2#newcode13
build/features_override.gypi:13: 'ENABLE_LINK_PREFETCH=1',
Totally optional suggestion:
After my experience with ENABLE_WEB_TIMING, I'd recommend checking this in as 0
and flipping to 1 in a subsequent change immediately after. The problem I ran
into is that for the builders to pick up the change, they all have to be
clobbered manually. If any tests expectations change as a result of the flip,
then it looks like your change broke the tests. If you isolate enabling into one
patch, it is more clear to sheriffs that it just needs a clobber.

http://codereview.chromium.org/3050016/diff/1/3
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3050016/diff/1/3#newcode128
chrome/browser/renderer_host/resource_dispatcher_host.cc:128:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisablePrefetch))
While I can't imagine that the HasSwitch call is that expensive (lower a string
and then a hash lookup), I've been asked to cache it when it is checked on a
per-request basis like this. Something like:
http://www.google.com/codesearch/p?hl=en#hfE6470xZHk/chrome/renderer/renderer...

http://codereview.chromium.org/3050016/diff/1/4
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3050016/diff/1/4#newcode181
chrome/common/chrome_switches.cc:181: const char kDisablePrefetch[]             
 = "disable-prefetch";
All the other switches have a comment here. You might want to indicate here
whether this is just for <link rel=prefetch> or also for x-subresource, etc.

http://codereview.chromium.org/3050016/diff/1/5
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/3050016/diff/1/5#newcode67
chrome/common/chrome_switches.h:67: extern const char kDisablePrefetch[];
Alphabetize one line lower

http://codereview.chromium.org/3050016/diff/1/9
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/1/9#newcode242
net/spdy/spdy_framer.h:242: // prefetches into one).
We should run this folding logic by mbelshe or willchan. My thoughts:
1. Is it possible to add a new SPDY priority rather than fold?
2. If we must fold, then based on the types of resources in
ResourceDispatcherHost::DetermineRequestPriority, it looks like it would be
better to fold LOW and LOWEST than to fold LOWEST and IDLE.

[cbentzel, 2010-07-26 17:06:32]

http://codereview.chromium.org/3050016/diff/1/9
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/1/9#newcode242
net/spdy/spdy_framer.h:242: // prefetches into one).
On 2010/07/26 16:15:37, tonyg wrote:
> We should run this folding logic by mbelshe or willchan. My thoughts:
> 1. Is it possible to add a new SPDY priority rather than fold?

That will happen with next version of SPDY.

[cbentzel, 2010-07-26 17:18:59]

LGTM, assuming you add some unit tests (RequestPriority -> SPDY priority being
an easy one, others less so).

What priority are requests such as safe browsing? Should these be moved to Idle?
Probably not as part of this CL, but worth looking at what else could move to
this new priority.

http://codereview.chromium.org/3050016/diff/1/2
File build/features_override.gypi (right):

http://codereview.chromium.org/3050016/diff/1/2#newcode13
build/features_override.gypi:13: 'ENABLE_LINK_PREFETCH=1',
Nit: These appear to be alphabetically sorted, make sure ENABLE_LINK_PREFETCH is
as well.

http://codereview.chromium.org/3050016/diff/1/7
File net/base/request_priority.h (left):

http://codereview.chromium.org/3050016/diff/1/7#oldcode23
net/base/request_priority.h:23: 
Nit: cpplint _expects_ this last empty newline.

http://codereview.chromium.org/3050016/diff/1/8
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/3050016/diff/1/8#newcode86
net/spdy/spdy_framer.cc:86: DCHECK(net::HIGHEST <= priority && priority <=
net::NUM_PRIORITIES);
< net::NUM_PRIORITIES, not <=

http://codereview.chromium.org/3050016/diff/1/9
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/1/9#newcode21
net/spdy/spdy_framer.h:21: #include "net/base/request_priority.h"
Do you need to actually include this? Is RequestPriority an enum? If so, I hate
lack of ability to forward declare enums, even if I understand the rationale.

http://codereview.chromium.org/3050016/diff/1/10
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3050016/diff/1/10#newcode387
net/spdy/spdy_session.cc:387: priority < net::NUM_PRIORITIES);
Nit: could move back to previous line.

[gavinp, 2010-07-29 15:22:59]

This latest upload adds a lot more tests, and remediates to review.

The functional test is kind of interesting: I think the idiom of a test starting
a webserver on any available port, and communicating that back to the client
would ultimately make system tests less flakey.

http://codereview.chromium.org/3050016/diff/1/2
File build/features_override.gypi (right):

http://codereview.chromium.org/3050016/diff/1/2#newcode13
build/features_override.gypi:13: 'ENABLE_LINK_PREFETCH=1',
On 2010/07/26 16:15:37, tonyg wrote:
> Totally optional suggestion:
> After my experience with ENABLE_WEB_TIMING, I'd recommend checking this in as
0
> and flipping to 1 in a subsequent change immediately after. The problem I ran
> into is that for the builders to pick up the change, they all have to be
> clobbered manually. If any tests expectations change as a result of the flip,
> then it looks like your change broke the tests. If you isolate enabling into
one
> patch, it is more clear to sheriffs that it just needs a clobber.

Done.

http://codereview.chromium.org/3050016/diff/1/2#newcode13
build/features_override.gypi:13: 'ENABLE_LINK_PREFETCH=1',
On 2010/07/26 17:18:59, cbentzel wrote:
> Nit: These appear to be alphabetically sorted, make sure ENABLE_LINK_PREFETCH
is
> as well.

Done.

http://codereview.chromium.org/3050016/diff/1/3
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3050016/diff/1/3#newcode128
chrome/browser/renderer_host/resource_dispatcher_host.cc:128:
CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisablePrefetch))
On 2010/07/26 16:15:37, tonyg wrote:
> While I can't imagine that the HasSwitch call is that expensive (lower a
string
> and then a hash lookup), I've been asked to cache it when it is checked on a
> per-request basis like this.

This is not in keeping with the existing style in resource_dispatcher_host,
there's numerous direct lookups of flags throughout the file.  I left it as is
to stay consistant.

http://codereview.chromium.org/3050016/diff/1/4
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3050016/diff/1/4#newcode181
chrome/common/chrome_switches.cc:181: const char kDisablePrefetch[]             
 = "disable-prefetch";
On 2010/07/26 16:15:37, tonyg wrote:
> All the other switches have a comment here. You might want to indicate here
> whether this is just for <link rel=prefetch> or also for x-subresource, etc.

Done.  Also alphabetised.

http://codereview.chromium.org/3050016/diff/1/5
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/3050016/diff/1/5#newcode67
chrome/common/chrome_switches.h:67: extern const char kDisablePrefetch[];
On 2010/07/26 16:15:37, tonyg wrote:
> Alphabetize one line lower

Done.

http://codereview.chromium.org/3050016/diff/1/7
File net/base/request_priority.h (left):

http://codereview.chromium.org/3050016/diff/1/7#oldcode23
net/base/request_priority.h:23: 
On 2010/07/26 17:18:59, cbentzel wrote:
> Nit: cpplint _expects_ this last empty newline.

I think cpplint only expects a newline on the last line, not a blank line at the
end?

http://codereview.chromium.org/3050016/diff/1/8
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/3050016/diff/1/8#newcode86
net/spdy/spdy_framer.cc:86: DCHECK(net::HIGHEST <= priority && priority <=
net::NUM_PRIORITIES);
On 2010/07/26 17:18:59, cbentzel wrote:
> < net::NUM_PRIORITIES, not <=

Done.

http://codereview.chromium.org/3050016/diff/1/9
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/1/9#newcode21
net/spdy/spdy_framer.h:21: #include "net/base/request_priority.h"
On 2010/07/26 17:18:59, cbentzel wrote:
> Do you need to actually include this? Is RequestPriority an enum? If so, I
hate
> lack of ability to forward declare enums, even if I understand the rationale.

Yes, it is.  I agree it's annoying.

http://codereview.chromium.org/3050016/diff/1/9#newcode242
net/spdy/spdy_framer.h:242: // prefetches into one).
On 2010/07/26 16:15:37, tonyg wrote:
> 2. If we must fold, then based on the types of resources in
> ResourceDispatcherHost::DetermineRequestPriority, it looks like it would be
> better to fold LOW and LOWEST than to fold LOWEST and IDLE.

You're right, Done.

http://codereview.chromium.org/3050016/diff/1/10
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3050016/diff/1/10#newcode387
net/spdy/spdy_session.cc:387: priority < net::NUM_PRIORITIES);
On 2010/07/26 17:18:59, cbentzel wrote:
> Nit: could move back to previous line.

Done.

[Mike Belshe, 2010-07-29 16:15:39]

sorry to ask you to move the priority function!!!

otherwise LGTM
Mike

http://codereview.chromium.org/3050016/diff/7003/14005
File chrome/test/functional/prefetch.py (right):

http://codereview.chromium.org/3050016/diff/7003/14005#newcode5
chrome/test/functional/prefetch.py:5: 
nit:  I can't tell from the name of this file whether this is a client or a
server or what it is used for.

If you could add a bunch of comments about what this does and how to use it,
that would be awesome.

http://codereview.chromium.org/3050016/diff/7003/14009
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/3050016/diff/7003/14009#newcode82
net/spdy/spdy_framer.cc:82: // TODO(gavinp): re-adjust this once SPDY v3 has
three priority bits,
I don't like this much - but lets leave it for now (although move out of
spdy_framer.*).  In draft3 we plan to add an extra bit for priority.

http://codereview.chromium.org/3050016/diff/7003/14010
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/7003/14010#newcode22
net/spdy/spdy_framer.h:22: #include "net/base/request_priority.h"
The spdy_framer.h/.cc are shared between the client and server.  I'd prefer to
move chrome-specific priority logic into the HttpNetworkTransaction or
SpdySession or anywhere but here :-)

http://codereview.chromium.org/3050016/diff/7003/14010#newcode244
net/spdy/spdy_framer.h:244: static int ConvertRequestPriorityToSpdyPriority(
so - let's move this function elsewhere

http://codereview.chromium.org/3050016/diff/7003/14011
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3050016/diff/7003/14011#newcode296
net/spdy/spdy_network_transaction_unittest.cc:296:
reinterpret_cast<spdy::SpdySynStreamControlFrame*>(req.get())->priority();
style: 80cols

http://codereview.chromium.org/3050016/diff/7003/14012
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3050016/diff/7003/14012#newcode387
net/spdy/spdy_session.cc:387: DCHECK(priority >= net::HIGHEST && priority <
net::NUM_PRIORITIES);
this is a good fix, thanks.  The RequestPriority really isn't a spdy priority.

[gavinp, 2010-07-29 18:59:29]

I moved ConvertRequestPriorityToSpdyPriority into http_network_transaction, but
left it just in namespace net.

http://codereview.chromium.org/3050016/diff/7003/14005
File chrome/test/functional/prefetch.py (right):

http://codereview.chromium.org/3050016/diff/7003/14005#newcode5
chrome/test/functional/prefetch.py:5: 
On 2010/07/29 16:15:39, Mike Belshe wrote:
> nit:  I can't tell from the name of this file whether this is a client or a
> server or what it is used for.
> 
> If you could add a bunch of comments about what this does and how to use it,
> that would be awesome.

Done.

http://codereview.chromium.org/3050016/diff/7003/14010
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/7003/14010#newcode22
net/spdy/spdy_framer.h:22: #include "net/base/request_priority.h"
On 2010/07/29 16:15:39, Mike Belshe wrote:
> The spdy_framer.h/.cc are shared between the client and server.  I'd prefer to
> move chrome-specific priority logic into the HttpNetworkTransaction or
> SpdySession or anywhere but here :-)

Done.

http://codereview.chromium.org/3050016/diff/7003/14011
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3050016/diff/7003/14011#newcode296
net/spdy/spdy_network_transaction_unittest.cc:296:
reinterpret_cast<spdy::SpdySynStreamControlFrame*>(req.get())->priority();
On 2010/07/29 16:15:39, Mike Belshe wrote:
> style: 80cols

Done.

[Mike Belshe, 2010-07-29 19:36:16]

http://codereview.chromium.org/3050016/diff/19001/20005
File chrome/test/functional/prefetch.py (right):

http://codereview.chromium.org/3050016/diff/19001/20005#newcode8
chrome/test/functional/prefetch.py:8: # and the tests assert that the webserver
logs reflect that.
This is very cool.

http://codereview.chromium.org/3050016/diff/19001/20012
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/19001/20012#newcode23
net/spdy/spdy_framer.h:23: #include "net/spdy/spdy_protocol.h"
I think you can revert this file to its original form now? (and remove from the
CL?) 

Same for spdy_framer.cc?

[Mike Belshe, 2010-07-29 19:36:34]

lgtm with removal of spdy_framer.cc/h.

[cbentzel, 2010-07-30 03:13:20]

LGTM!

http://codereview.chromium.org/3050016/diff/19001/20005
File chrome/test/functional/prefetch.py (right):

http://codereview.chromium.org/3050016/diff/19001/20005#newcode83
chrome/test/functional/prefetch.py:83: var para =
document.getElementById("firstparagraph");
Which element is "firstparagraph"? The only id I see is the "p1" below.

http://codereview.chromium.org/3050016/diff/19001/20005#newcode111
chrome/test/functional/prefetch.py:111: server = HTTPServer(('',0),
PrefetchServerHandler)
Does the ('', 0) address cause it to establish a listening socket on an
ephemeral port?

http://codereview.chromium.org/3050016/diff/19001/20005#newcode129
chrome/test/functional/prefetch.py:129: self.assertEqual(True,
server_log.isRetrieved("static-prefetch-target.html"))
Nitty-nit: 80-char line issues.

http://codereview.chromium.org/3050016/diff/19001/20005#newcode133
chrome/test/functional/prefetch.py:133: web_server =
multiprocessing.Process(target=run_web_server,args=(queue,))
Ah, cool. I haven't really used python2.6 yet so I would have defaulted to
subprocess. multiprocesing looks like a great choice.

[gavinp, 2010-07-30 15:14:45]

thanks for the reviews.  with no objections and a try-blessing, i'll land this
version today.

http://codereview.chromium.org/3050016/diff/19001/20005
File chrome/test/functional/prefetch.py (right):

http://codereview.chromium.org/3050016/diff/19001/20005#newcode83
chrome/test/functional/prefetch.py:83: var para =
document.getElementById("firstparagraph");
On 2010/07/30 03:13:20, cbentzel wrote:
> Which element is "firstparagraph"? The only id I see is the "p1" below. 

Fixed, should have been p1.

http://codereview.chromium.org/3050016/diff/19001/20005#newcode111
chrome/test/functional/prefetch.py:111: server = HTTPServer(('',0),
PrefetchServerHandler)
On 2010/07/30 03:13:20, cbentzel wrote:
> Does the ('', 0) address cause it to establish a listening socket on an
> ephemeral port? 

Yup.

http://codereview.chromium.org/3050016/diff/19001/20005#newcode129
chrome/test/functional/prefetch.py:129: self.assertEqual(True,
server_log.isRetrieved("static-prefetch-target.html"))
On 2010/07/30 03:13:20, cbentzel wrote:
> Nitty-nit: 80-char line issues.

Done.

http://codereview.chromium.org/3050016/diff/19001/20005#newcode133
chrome/test/functional/prefetch.py:133: web_server =
multiprocessing.Process(target=run_web_server,args=(queue,))
On 2010/07/30 03:13:20, cbentzel wrote:
> Ah, cool. I haven't really used python2.6 yet so I would have defaulted to
> subprocess. multiprocesing looks like a great choice.

After I realised the global interpreter lock was going to hurt me for threads, I
was really happy to find multiprocessing.

http://codereview.chromium.org/3050016/diff/19001/20012
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3050016/diff/19001/20012#newcode23
net/spdy/spdy_framer.h:23: #include "net/spdy/spdy_protocol.h"
On 2010/07/29 19:36:16, Mike Belshe wrote:
> I think you can revert this file to its original form now? (and remove from
the
> CL?) 
> 
> Same for spdy_framer.cc?

Done.

[jar (doing other things), 2010-07-31 02:08:18]

I don't know why I didn't see this review earlier... my name seems to be on the
reviewers list... but none of the review  emails were sent to me :-(.  I'll take
a wild/silly guess that only saying "jar" is not enough and you need "jar at
chromium.org."  oh well...

That said...

The biggest thing that I think is missing from this landing is any way to
monitor results.  At a minimum, it would be good to understand how often this
feature is getting used, probably as compared with some other activity.  For
example, you could normalize relative to page loads, or relative to network
transactions (which include subresources).  You might also want to normalize
prefetched bytes vs overall fetched bytes.

Better yet, it would be good to understand how often a resource was fetched, and
then never used (because it was evicted, its cached version proved unusable, or
because it could not be cached for some reason), vs how often it ended up being
used.

Finally, it would be great to set up an A/B experiment to try to eval the impact
on PLT (Page Load Time).  Prefetching of resources can help if we end up going
to a page, but will hurt if they clog our pipes while we're trying to go
elsewhere (or if they clog our pipes while we're trying to load the rest of the
current page).

One of the weaknesses in the current server side support for this feature in
that there is no feedback as to overall effectiveness, and hence it is difficult
to optimize thresholds etc.  IMO, one of the greatest benefits of implementing
this is to supply feedback, and gather some quantitative data.

Bottom line: You need to add several of histograms, and probably a field-trial
test to modulate some PLT histograms.

[gavinp, 2010-07-31 02:29:22]

Jim, I agree completely.

My view is that prefetching is only tentatively in M7 until we get results; I
want to land histograms and an experiment as quickly as possible.  If we don't
see solid results in an experiment (using histograms similar to what we
discussed earlier), then I think we need to revisit the default of using
prefetching.

- Gavin

On 2010/07/31 02:08:18, jar wrote:
> I don't know why I didn't see this review earlier... my name seems to be on
the
> reviewers list... but none of the review  emails were sent to me :-(.  I'll
take
> a wild/silly guess that only saying "jar" is not enough and you need "jar at
> chromium.org."  oh well...
> 
> That said...
> 
> The biggest thing that I think is missing from this landing is any way to
> monitor results.  At a minimum, it would be good to understand how often this
> feature is getting used, probably as compared with some other activity.  For
> example, you could normalize relative to page loads, or relative to network
> transactions (which include subresources).  You might also want to normalize
> prefetched bytes vs overall fetched bytes.
> 
> Better yet, it would be good to understand how often a resource was fetched,
and
> then never used (because it was evicted, its cached version proved unusable,
or
> because it could not be cached for some reason), vs how often it ended up
being
> used.
> 
> Finally, it would be great to set up an A/B experiment to try to eval the
impact
> on PLT (Page Load Time).  Prefetching of resources can help if we end up going
> to a page, but will hurt if they clog our pipes while we're trying to go
> elsewhere (or if they clog our pipes while we're trying to load the rest of
the
> current page).
> 
> One of the weaknesses in the current server side support for this feature in
> that there is no feedback as to overall effectiveness, and hence it is
difficult
> to optimize thresholds etc.  IMO, one of the greatest benefits of implementing
> this is to supply feedback, and gather some quantitative data.
> 
> Bottom line: You need to add several of histograms, and probably a field-trial
> test to modulate some PLT histograms.