Support speculative pre-connection to search URLs

Support speculative pre-connection to search URLs

Implement several flavors of TCP/IP speculative preconnection
under a command line flag (not yet on by default).

The first area of preconnection takes place when a user types
a query into the omnibox, as we preconnect to the search service
when the omnibox suggests it is going to do a search.

The second area involves subresources, such as images.
When a navigation takes place, and we've seen navigations
to that domain/port before, and the history-based 
probabability that we'll need to make a connection to
a second site (host/port) is sufficiently large, then we
preconnect to that second site while we are still connecting
to the primary site (and before we've gotten content from
the primary site.

We also fall-back to mere DNS pre-resolution of subresource
hostnames when the probability of a connection to the
subresource is not high enough.

BUG=42694
r=pkasting,willchan,mbelshe

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

[jar (doing other things), 2010-04-28 01:23:03]

This is mostly working I think (I still need some more unit tests, and a little
more cleanup).

Pkasting: Can you review changes in autocomplete.*

WillChan: Can you review the new preconnnect.cc file, which tries to use your
API?  You suggested you'd have comments about how to deal with proxies as well.

Mike: You can hold off on a more global review, as I'll do a bit more cleaning. 
You might comment on constants in the algorithm in referrer.cc (see comment at
head of file), where we develop expected connection rates for subresources.

Thanks,

Jim

[Peter Kasting, 2010-04-28 05:08:06]

http://codereview.chromium.org/1585029/diff/55008/94004
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/1585029/diff/55008/94004#newcode376
chrome/browser/autocomplete/autocomplete.h:376: // The type of this match is a
search query with the default engine.
I'd prefer to keep the API surface here smaller rather than larger.  Just
directly compare |type| to SEARCH_WHAT_YOU_TYPED, SEARCH_HISTORY, or
SEARCH_SUGGEST -- it's public.

[willchan no longer on Chromium, 2010-04-28 18:23:10]

[ cc: eroman ]

General comments:
* You're making changes to the DNS prefetching system at the same time.  Perhaps
it'd be nicer to make them first, separate from this changelist, since they
might have a performance impact that you may want to quantify via A/B tests.  In
particular, you seem to add a heuristic to prevent doing too many DNS prefetches
too quickly for the omnibox.

* For ClientSocketHandle, I commented that you don't need to actually keep it
alive for the callback.  You should call Init() and the forget about it (just
destroy the request.  if there was space for a job, it'll get fired.  if you hit
a limit and a job wasn't created, we probably shouldn't bother fighting for it
anyway, so just let the job not get created for now).

* I didn't see how you would handle the transition of profiles.  Will this
change safely handle the transition of your preference format for
serializing/deserializing the dns prefetch list?  I didn't read that too
carefully.

* To get this to work for proxies, we should probably share the code in
HttpNetworkTransaction::DoInitConnection().  wtc was talking about extracting
it.  I am touching that code right now in a separate changelist.  If wtc doesn't
extract it, I will, and then you should use it.  That'll fix the proxy case. 
It's also fine to leave it as a TODO for now.  It's important to reuse the same
code, since if we change the connection_group names or what not in the future,
then we don't want to break this code here.

http://codereview.chromium.org/1585029/diff/55008/94022
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94022#newcode1063
chrome/browser/browser_main.cc:1063: chrome_browser_net::DnsGlobalInit
dns_prefetch(user_prefs, local_state,
parameters must be aligned with first parameter (|users_prefs| here).  You
should probably move that down to the next line.

http://codereview.chromium.org/1585029/diff/55008/94007
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94007#newcode136
chrome/browser/net/dns_global.cc:136: static base::TimeTicks
last_tcp_warm_for_host;
You don't seem to use last_tcp_warm_for_host.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode139
chrome/browser/net/dns_global.cc:139: const int kMinPreresolveSeconds(10);
Where are you getting this heuristic from?  Are doing any A/B tests for these
changes to stop doing DNS prefetching in certain cases?

http://codereview.chromium.org/1585029/diff/55008/94007#newcode145
chrome/browser/net/dns_global.cc:145: net::HostPortPair hostport(url.host(),
url.EffectiveIntPort());
You should always use HostNoBrackets() with HostPortPair.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode149
chrome/browser/net/dns_global.cc:149: const int kMaxSearchKeepaliveSeconds(30);
Where does this keep alive number come from?

http://codereview.chromium.org/1585029/diff/55008/94007#newcode255
chrome/browser/net/dns_global.cc:255:
navigation_info.SetHostname(net::HostPortPair(request_info.hostname().c_str(),
I don't think you need the c_str() here.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode323
chrome/browser/net/dns_global.cc:323: if
(first_resolutions_.find(navigation_info.hostport()) !=
Oftentimes, I use ContainsKey() (defined in stl_util-inl.h) for this, as I find
it more readable that a comparison to the end iterator.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode336
chrome/browser/net/dns_global.cc:336: it++) {
this should be ++it

http://codereview.chromium.org/1585029/diff/55008/94016
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94016#newcode575
chrome/browser/net/dns_master.cc:575: DCHECK(false);
NOTREACHED()?

http://codereview.chromium.org/1585029/diff/55008/94016#newcode585
chrome/browser/net/dns_master.cc:585: DCHECK(false);
NOTREACHED()?

http://codereview.chromium.org/1585029/diff/55008/94016#newcode589
chrome/browser/net/dns_master.cc:589: DCHECK(false);
NOTREACHED()?

http://codereview.chromium.org/1585029/diff/55008/94016#newcode595
chrome/browser/net/dns_master.cc:595: DCHECK(false);
NOTREACHED()?

http://codereview.chromium.org/1585029/diff/55008/94025
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94025#newcode33
chrome/browser/net/dns_master_unittest.cc:33: typedef net::HostPortPair
HostPort;
I don't know if I like this typedef.  I think if you want to shorten it, you
should just doing using net::HostPortPair.

http://codereview.chromium.org/1585029/diff/55008/94008
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94008#newcode13
chrome/browser/net/preconnect.cc:13:
Preconnect::Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
You don't really need this object.  You can just make this a function. 
Instantiate the ClientSocketHandle on the stack.  After you call Init() on it,
just let it go out of scope.  You don't need to wait for the callback.  Init()
will create a TCPConnectJob as long as we don't hit any limits (connections per
host or total tcp connections).  When ClientSocketHandle is destroyed, it'll
invoke Reset() which will, if the job succeeded synchronously, return the socket
back to the pool.  If it failed synchronously, it does nothing.  If it is
ERR_IO_PENDING, then it'll destroy the request, but not the TCPConnectJob (which
is late bound anyway, so it won't get cancelled, but will instead sit around in
the idle socket pool whenever it completes successfully).

http://codereview.chromium.org/1585029/diff/55008/94008#newcode21
chrome/browser/net/preconnect.cc:21: int rv = handle_.Init("http://" +
hostport.ToString() , params, net::LOWEST,
You don't need the http:// anymore.

http://codereview.chromium.org/1585029/diff/55008/94008#newcode52
chrome/browser/net/preconnect.cc:52: DCHECK(false);
I often LOG(DFATAL) in these cases.  Then we get ERROR messages in release mode
and crashes in debug mode.

http://codereview.chromium.org/1585029/diff/55008/94010
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/55008/94010#newcode1
chrome/browser/net/preconnect.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
This is out of date :)

http://codereview.chromium.org/1585029/diff/55008/94010#newcode41
chrome/browser/net/preconnect.h:41: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/1585029/diff/55008/94010#newcode42
chrome/browser/net/preconnect.h:42: }  // chrome_browser_net
You should probably add a newline before the closure of the namespace to be
consistent with what you have above with a newline after the opening of the
namespace.

http://codereview.chromium.org/1585029/diff/55008/94015
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94015#newcode14
chrome/browser/net/referrer.cc:14: // Smoothing paramater for updating
subresource_use_rate_.
s/paramater/parameter/

http://codereview.chromium.org/1585029/diff/55008/94011
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1585029/diff/55008/94011#newcode26
net/base/host_port_pair.h:26: // Equality test of contents. (Probably another
violation of style guide).
Your above comment on operator< is correct, but Equals() is recommended by the
style guide (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...).

[eroman, 2010-04-28 20:25:19]

I haven't read this change yet, but my initial concern is how this is going to
interact with the global socket limits.

There is a relatively small bound on the total number of sockets that chrome
will keep open. For proxies this is even smaller (15 total!).

If we aren't *very* careful, pre-warming might be evicting more useful idle
sockets from our small pool.

[willchan no longer on Chromium, 2010-04-28 20:28:20]

On Wed, Apr 28, 2010 at 1:25 PM,  <[email protected]> wrote:
> I haven't read this change yet, but my initial concern is how this is going
> to
> interact with the global socket limits.

This is a very good point.  The current change is behind a command
line flag, and clearly needs tweaking to handle this situation.  We
should run experiments.  Jim, note that I am planning on running
experiments with idle socket limits on dev channel in Chrome 6.  Our
experiments won't be orthogonal, so we have to be careful about not
smashing each others' experiments.

>
> There is a relatively small bound on the total number of sockets that chrome
> will keep open. For proxies this is even smaller (15 total!).
>
> If we aren't *very* careful, pre-warming might be evicting more useful idle
> sockets from our small pool.
>
> http://codereview.chromium.org/1585029/show
>

[mbelshe, 2010-04-29 02:33:06]

On Wed, Apr 28, 2010 at 1:28 PM, William Chan (陈智昌)
<[email protected]>wrote:

> On Wed, Apr 28, 2010 at 1:25 PM,  <[email protected]> wrote:
> > I haven't read this change yet, but my initial concern is how this is
> going
> > to
> > interact with the global socket limits.
>
> This is a very good point.


I'm not sure I follow- the 100 socket limit is pretty gratuitous, right?
 (recently bumped to 256)

Why do we have a limit again?

Mike



>  The current change is behind a command
> line flag, and clearly needs tweaking to handle this situation.  We
> should run experiments.  Jim, note that I am planning on running
> experiments with idle socket limits on dev channel in Chrome 6.  Our
> experiments won't be orthogonal, so we have to be careful about not
> smashing each others' experiments.
>


> >
> > There is a relatively small bound on the total number of sockets that
> chrome
> > will keep open. For proxies this is even smaller (15 total!).
> >
> > If we aren't *very* careful, pre-warming might be evicting more useful
> idle
> > sockets from our small pool.
> >
> > http://codereview.chromium.org/1585029/show
> >
>

[Peter Kasting, 2010-04-29 23:55:13]

http://codereview.chromium.org/1585029/diff/55008/94004
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/1585029/diff/55008/94004#newcode376
chrome/browser/autocomplete/autocomplete.h:376: // The type of this match is a
search query with the default engine.
On 2010/04/28 05:08:06, Peter Kasting wrote:
> I'd prefer to keep the API surface here smaller rather than larger.  Just
> directly compare |type| to SEARCH_WHAT_YOU_TYPED, SEARCH_HISTORY, or
> SEARCH_SUGGEST -- it's public.

Let me be clearer about why I think this is OK.

You have a single usage.  That usage is inside the Autocomplete system -- it's
in AutocompleteEditModel.cc.  If you had multiple uses, a utility function would
make sense.  If you had uses outside Autocomplete code, an abstraction that
hides the match types would perhaps make sense, since external code shouldn't
necessarily be expected to understand the meanings of these enum values.  Since
neither of these is true, inlining the code in the caller is the simplest way to
go.

If you're going to do this at the caller side, then a conditional is very short,
whereas a switch is very long.  When the determination you need to make is a
simple boolean, it is much faster for one's eye to scan a conditional than a
switch.

If we needed every usage of an enum element to be safed against future changes
in such a way that adding a new element caused a compile failure at each
location, working with enums would be ridiculously cumbersome.  So deciding when
this is truly valuable is actually meaningful.  In your particular case, you
have an uncommon question you're answering, and the consequences of a wrong
answer are fairly minor.  The penalty for "getting this wrong" is low.  Given
that anyone adding a new SEARCH_ type would need to grep for the existing types
anyway, it is unlikely that we would even get this wrong.  Thus, the maintenance
benefit of a switch over a conditional is marginal for this case.

[jar (doing other things), 2010-04-30 02:00:04]

Response to willchan's "general comments"::

On Wed, Apr 28, 2010 at 11:23 AM, <[email protected]> wrote:

> General comments:
> * You're making changes to the DNS prefetching system at the same time.
>  Perhaps
> it'd be nicer to make them first, separate from this changelist, since they
> might have a performance impact that you may want to quantify via A/B
> tests.  In
> particular, you seem to add a heuristic to prevent doing too many DNS
> prefetches
> too quickly for the omnibox.
>
>
I think this was a relatively tiny change, with no foreseeable consequences
except for a slight reduction in load (on the DNS resolver). Inline comments
on this point will follow with the list of CL comment responses.


> * For ClientSocketHandle, I commented that you don't need to actually keep
> it
> alive for the callback.  You should call Init() and the forget about it
> (just
> destroy the request.  if there was space for a job, it'll get fired.  if
> you hit
> a limit and a job wasn't created, we probably shouldn't bother fighting for
> it
> anyway, so just let the job not get created for now).
>

I need to chat with you about this.  Again, I'll put inline comments on this
issue since you also pulled it out below.


>
> * I didn't see how you would handle the transition of profiles.  Will this
> change safely handle the transition of your preference format for
> serializing/deserializing the dns prefetch list?  I didn't read that too
> carefully.
>

That was one of the details that had to be handled.  The code already was
defensive in its deserialization, so it would be safe (although it would
DCHECK if you go backwards a rev).  I've now explicitly added versioning
data to each persistent save, so it will (in the future) be even safe to
move forward and backwards across versions (the wrong-version data will be
discarded transparently... which is the "right" (safe?) thing to do with
accumulated learned data).


>
> * To get this to work for proxies, we should probably share the code in
> HttpNetworkTransaction::DoInitConnection().  wtc was talking about
> extracting
> it.  I am touching that code right now in a separate changelist.  If wtc
> doesn't
> extract it, I will, and then you should use it.  That'll fix the proxy
> case.
> It's also fine to leave it as a TODO for now.  It's important to reuse the
> same
> code, since if we change the connection_group names or what not in the
> future,
> then we don't want to break this code here.
>
>
I expect it to stay as a TODO for now.


Other details to follow shortly.

Jim

[jar (doing other things), 2010-04-30 02:02:27]

Response to all of willchan and pkasting comments.

I've also done a pile of "hand linting" and put in support for versioning of
persistent data.

http://codereview.chromium.org/1585029/diff/55008/94004
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/1585029/diff/55008/94004#newcode376
chrome/browser/autocomplete/autocomplete.h:376: // The type of this match is a
search query with the default engine.
Per your request, I pulled the info about this enum into the DNS code, and out
of your class.

I couldn't justify inlining, as a helper function is much more readable.  I
listed the enums explicitly so that there would be a compile warning if the enum
list changes.  For optimization code (such as pre-connections), these
optimizations are generally fragile, and can be broken without anyone detecting
it, so I'd prefer to make it harder to break the optimization across an
interface (such as by adding new enumerated states).  This also makes it much
clear what types of matches are being serviced by preconnection, and what types
of URLs aren't.


On 2010/04/29 23:55:13, Peter Kasting wrote:
> On 2010/04/28 05:08:06, Peter Kasting wrote:
> > I'd prefer to keep the API surface here smaller rather than larger.  Just
> > directly compare |type| to SEARCH_WHAT_YOU_TYPED, SEARCH_HISTORY, or
> > SEARCH_SUGGEST -- it's public.
> 
> Let me be clearer about why I think this is OK.
> 
> You have a single usage.  That usage is inside the Autocomplete system -- it's
> in AutocompleteEditModel.cc.  If you had multiple uses, a utility function
would
> make sense.  If you had uses outside Autocomplete code, an abstraction that
> hides the match types would perhaps make sense, since external code shouldn't
> necessarily be expected to understand the meanings of these enum values. 
Since
> neither of these is true, inlining the code in the caller is the simplest way
to
> go.
> 
> If you're going to do this at the caller side, then a conditional is very
short,
> whereas a switch is very long.  When the determination you need to make is a
> simple boolean, it is much faster for one's eye to scan a conditional than a
> switch.
> 
> If we needed every usage of an enum element to be safed against future changes
> in such a way that adding a new element caused a compile failure at each
> location, working with enums would be ridiculously cumbersome.  So deciding
when
> this is truly valuable is actually meaningful.  In your particular case, you
> have an uncommon question you're answering, and the consequences of a wrong
> answer are fairly minor.  The penalty for "getting this wrong" is low.  Given
> that anyone adding a new SEARCH_ type would need to grep for the existing
types
> anyway, it is unlikely that we would even get this wrong.  Thus, the
maintenance
> benefit of a switch over a conditional is marginal for this case.

http://codereview.chromium.org/1585029/diff/55008/94022
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94022#newcode1063
chrome/browser/browser_main.cc:1063: chrome_browser_net::DnsGlobalInit
dns_prefetch(user_prefs, local_state,
On 2010/04/28 18:23:11, willchan wrote:
> parameters must be aligned with first parameter (|users_prefs| here).  You
> should probably move that down to the next line.

Done.

http://codereview.chromium.org/1585029/diff/55008/94007
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94007#newcode136
chrome/browser/net/dns_global.cc:136: static base::TimeTicks
last_tcp_warm_for_host;
On 2010/04/28 18:23:11, willchan wrote:
> You don't seem to use last_tcp_warm_for_host.

Oops... I transitioned to a different static timer.
I deleted this static.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode139
chrome/browser/net/dns_global.cc:139: const int kMinPreresolveSeconds(10);
On 2010/04/28 18:23:11, willchan wrote:
> Where are you getting this heuristic from?

I just tried to land some reasonable protective constants.

>  Are doing any A/B tests for these
> changes to stop doing DNS prefetching in certain cases?

I haven't A/B tested this, but it only reduces load.  Personal recent testing
showed that the suggestions were coming in groups of two, only milliseconds
appart (on my machine).  It seemed that I should reduced the workload to
something reasonable by at least de-duping this here (and avoiding the numerous
calls, which would probably effectively de-dup this in the resolver, and
certainly waste resources getting to that point.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode145
chrome/browser/net/dns_global.cc:145: net::HostPortPair hostport(url.host(),
url.EffectiveIntPort());
On 2010/04/28 18:23:11, willchan wrote:
> You should always use HostNoBrackets() with HostPortPair.

I search for all places where I pulled out member |host|, and changed this.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode149
chrome/browser/net/dns_global.cc:149: const int kMaxSearchKeepaliveSeconds(30);
On 2010/04/28 18:23:11, willchan wrote:
> Where does this keep alive number come from?

This was another number I took a wild guess about.  Given that search URLs will
stay alive for some period of time (perhaps closer to 1-5 minutes), it seemed
that I shouldn't try to pre-warm a connection more often than every 30 seconds. 
I could probably use a longer period of time, but I just wanted to reduce load
on the rest of the system by deduping these calls below some interval.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode255
chrome/browser/net/dns_global.cc:255:
navigation_info.SetHostname(net::HostPortPair(request_info.hostname().c_str(),
On 2010/04/28 18:23:11, willchan wrote:
> I don't think you need the c_str() here.

Done.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode323
chrome/browser/net/dns_global.cc:323: if
(first_resolutions_.find(navigation_info.hostport()) !=
On 2010/04/28 18:23:11, willchan wrote:
> Oftentimes, I use ContainsKey() (defined in stl_util-inl.h) for this, as I
find
> it more readable that a comparison to the end iterator.

Done.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode336
chrome/browser/net/dns_global.cc:336: it++) {
On 2010/04/28 18:23:11, willchan wrote:
> this should be ++it

Done.

http://codereview.chromium.org/1585029/diff/55008/94016
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94016#newcode575
chrome/browser/net/dns_master.cc:575: DCHECK(false);
On 2010/04/28 18:23:11, willchan wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/1585029/diff/55008/94016#newcode585
chrome/browser/net/dns_master.cc:585: DCHECK(false);
On 2010/04/28 18:23:11, willchan wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/1585029/diff/55008/94016#newcode589
chrome/browser/net/dns_master.cc:589: DCHECK(false);
On 2010/04/28 18:23:11, willchan wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/1585029/diff/55008/94016#newcode595
chrome/browser/net/dns_master.cc:595: DCHECK(false);
On 2010/04/28 18:23:11, willchan wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/1585029/diff/55008/94025
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94025#newcode33
chrome/browser/net/dns_master_unittest.cc:33: typedef net::HostPortPair
HostPort;
On 2010/04/28 18:23:11, willchan wrote:
> I don't know if I like this typedef.  I think if you want to shorten it, you
> should just doing using net::HostPortPair.

Done.

http://codereview.chromium.org/1585029/diff/55008/94008
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94008#newcode13
chrome/browser/net/preconnect.cc:13:
Preconnect::Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
I'm anticipating putting some metrics in/around this point, and suspect it will
be easier with the callback (and memory costs etc. costs should be small
compared to the activity we instigate). 

I'm also a little wary that future optimizations may cause the TCPConnectJob,
but maybe my fears of this interface are not justified.


On 2010/04/28 18:23:11, willchan wrote:
> You don't really need this object.  You can just make this a function. 
> Instantiate the ClientSocketHandle on the stack.  After you call Init() on it,
> just let it go out of scope.  You don't need to wait for the callback.  Init()
> will create a TCPConnectJob as long as we don't hit any limits (connections
per
> host or total tcp connections).  When ClientSocketHandle is destroyed, it'll
> invoke Reset() which will, if the job succeeded synchronously, return the
socket
> back to the pool.  If it failed synchronously, it does nothing.  If it is
> ERR_IO_PENDING, then it'll destroy the request, but not the TCPConnectJob
(which
> is late bound anyway, so it won't get cancelled, but will instead sit around
in
> the idle socket pool whenever it completes successfully).

http://codereview.chromium.org/1585029/diff/55008/94008#newcode21
chrome/browser/net/preconnect.cc:21: int rv = handle_.Init("http://" +
hostport.ToString() , params, net::LOWEST,
On 2010/04/28 18:23:11, willchan wrote:
> You don't need the http:// anymore.

Done.

http://codereview.chromium.org/1585029/diff/55008/94008#newcode52
chrome/browser/net/preconnect.cc:52: DCHECK(false);
On 2010/04/28 18:23:11, willchan wrote:
> I often LOG(DFATAL) in these cases.  Then we get ERROR messages in release
mode
> and crashes in debug mode.

Done.

http://codereview.chromium.org/1585029/diff/55008/94010
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/55008/94010#newcode1
chrome/browser/net/preconnect.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2010/04/28 18:23:11, willchan wrote:
> This is out of date :)

Done.

http://codereview.chromium.org/1585029/diff/55008/94010#newcode41
chrome/browser/net/preconnect.h:41: };
On 2010/04/28 18:23:11, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/1585029/diff/55008/94010#newcode42
chrome/browser/net/preconnect.h:42: }  // chrome_browser_net
On 2010/04/28 18:23:11, willchan wrote:
> You should probably add a newline before the closure of the namespace to be
> consistent with what you have above with a newline after the opening of the
> namespace.

Lint complains when I have the "blank line at end of block.

http://codereview.chromium.org/1585029/diff/55008/94015
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94015#newcode14
chrome/browser/net/referrer.cc:14: // Smoothing paramater for updating
subresource_use_rate_.
On 2010/04/28 18:23:11, willchan wrote:
> s/paramater/parameter/

Done.

http://codereview.chromium.org/1585029/diff/55008/94011
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1585029/diff/55008/94011#newcode26
net/base/host_port_pair.h:26: // Equality test of contents. (Probably another
violation of style guide).
The issue is that this is a struct, not a class, and hence is not supposed to
have any methods other than accessors (I think).

On 2010/04/28 18:23:11, willchan wrote:
> Your above comment on operator< is correct, but Equals() is recommended by the
> style guide (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...).

[Peter Kasting, 2010-04-30 02:17:28]

r-.

http://codereview.chromium.org/1585029/diff/84004/111003
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/84004/111003#newcode690
chrome/browser/net/dns_global.cc:690: static bool
IsPreconnectableSearchType(const AutocompleteMatch& match) {
This is worse than your previous patch because it exposes knowledge of the
meanings of these types outside the Autocomplete system, which is the only place
that's supposed to know and understand what these mean.  This is effectively a
layering violation.

You may not check in with this.  Please do things the way I asked as I am the
author and maintainer of the Autocomplete system, and nothing I am asking you to
do violates the style guide.

[Peter Kasting, 2010-04-30 05:00:11]

On 2010/04/30 02:17:28, Peter Kasting wrote:
> You may not check in with this.  Please do things the way I asked as I am the
> author and maintainer of the Autocomplete system, and nothing I am asking you
to
> do violates the style guide.

BTW, lest this sound too harsh, I don't think your goals are stupid or wrong,
and I appreciate that you have readability and maintainability as priorities. 
It's just that I've spent several years of my life in the Autocomplete code, and
plan to spend most of M6 fixing bugs in it, so I have a pretty strong opinion
about where I'm going to rewrite, what the maintenance issues are, etc.  It's
not just a theoretical concern.  Please humor me :)

[jar (doing other things), 2010-05-04 01:32:58]

I think this may have addressed Pkasting's comments.

Mike: If you can review this, I'd appreciate it.

Will/Eroman: I think I've addressed your points (in previous CL commentary).  If
I haven't please remind me.

thanks,

Jim

http://codereview.chromium.org/1585029/diff/84004/111003
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/84004/111003#newcode690
chrome/browser/net/dns_global.cc:690: static bool
IsPreconnectableSearchType(const AutocompleteMatch& match) {
I originally tried to support encapsulation, and you objected.  It would appear
that this attempt to comply with your request and "use public data" externally
was more "external" than you had in mind.  

The newest patch goes back to preserving the encapsulation (as the original
patch did), but uses a static helper function (with file-static linkage) to
ensure that the API is not enlarged.

On 2010/04/30 02:17:29, Peter Kasting wrote:
> This is worse than your previous patch because it exposes knowledge of the
> meanings of these types outside the Autocomplete system, which is the only
place
> that's supposed to know and understand what these mean.  This is effectively a
> layering violation.
> 
> You may not check in with this.  Please do things the way I asked as I am the
> author and maintainer of the Autocomplete system, and nothing I am asking you
to
> do violates the style guide.

[Peter Kasting, 2010-05-04 02:01:16]

LGTM

http://codereview.chromium.org/1585029/diff/151002/146002
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1585029/diff/151002/146002#newcode574
chrome/browser/autocomplete/autocomplete_edit.cc:574:
UMA_HISTOGRAM_ENUMERATION("Autocomplete.MatchType", type,
Are you sure you want this here?  Observe() gets called a lot (multiple times
per keystroke), and if you note a separate data point each time, you may skew
your results.

http://codereview.chromium.org/1585029/diff/151002/146002#newcode577
chrome/browser/autocomplete/autocomplete_edit.cc:577: // These cases are for
default search service, specified by the user.
Nit: This comment doesn't seem to be accurate now that SEARCH_OTHER_ENGINE is in
the list.

http://codereview.chromium.org/1585029/diff/151002/146002#newcode578
chrome/browser/autocomplete/autocomplete_edit.cc:578: case
AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
Of these four types, this is the lowest-quality signal -- you'll get this for
all inputs that don't inline autocomplete and aren't a valid address on their
own, which might be a lot.  Dunno whether you always want to prewarm for it.

[willchan no longer on Chromium, 2010-05-04 02:16:54]

Oops, I noticed I didn't mail these out last week.  I haven't read it again to
see if it's up to date now that you've made some changes, but hopefully they're
still relevant.

http://codereview.chromium.org/1585029/diff/55008/94007
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94007#newcode139
chrome/browser/net/dns_global.cc:139: const int kMinPreresolveSeconds(10);
On 2010/04/30 02:02:27, jar wrote:
> On 2010/04/28 18:23:11, willchan wrote:
> > Where are you getting this heuristic from?
> 
> I just tried to land some reasonable protective constants.
> 
> >  Are doing any A/B tests for these
> > changes to stop doing DNS prefetching in certain cases?
> 
> I haven't A/B tested this, but it only reduces load.  Personal recent testing
> showed that the suggestions were coming in groups of two, only milliseconds
> appart (on my machine).  It seemed that I should reduced the workload to
> something reasonable by at least de-duping this here (and avoiding the
numerous
> calls, which would probably effectively de-dup this in the resolver, and
> certainly waste resources getting to that point.
> 

Do you think it makes a good perf impact?  If so, A/B testing would be nice.  If
you think it's too minor, then I'm fine just putting it in.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode149
chrome/browser/net/dns_global.cc:149: const int kMaxSearchKeepaliveSeconds(30);
On 2010/04/30 02:02:27, jar wrote:
> On 2010/04/28 18:23:11, willchan wrote:
> > Where does this keep alive number come from?
> 
> This was another number I took a wild guess about.  Given that search URLs
will
> stay alive for some period of time (perhaps closer to 1-5 minutes), it seemed
> that I shouldn't try to pre-warm a connection more often than every 30
seconds. 
> I could probably use a longer period of time, but I just wanted to reduce load
> on the rest of the system by deduping these calls below some interval.

I think comments on this could be useful.

http://codereview.chromium.org/1585029/diff/55008/94008
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94008#newcode13
chrome/browser/net/preconnect.cc:13:
Preconnect::Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
On 2010/04/30 02:02:27, jar wrote:
> I'm anticipating putting some metrics in/around this point, and suspect it
will
> be easier with the callback (and memory costs etc. costs should be small
> compared to the activity we instigate). 

Fair enough.  You must be careful here.  Remember, this is the late binding API.
 So, you will get the first available socket according to your priority.  It
won't necessarily exactly match the socket that you're trying to prewarm.  If
you need something like that, it might be better to add something explicitly
into the ClientSocketHandle/ClientSocketPool API so that the ClientSocketPool
itself can do the metric tracking.  This is what we do for NetLog tracing of
backup connect jobs for the dropped SYNs.

> 
> I'm also a little wary that future optimizations may cause the TCPConnectJob,
> but maybe my fears of this interface are not justified.

I think you might have dropped a few words here.

> 
> 
> On 2010/04/28 18:23:11, willchan wrote:
> > You don't really need this object.  You can just make this a function. 
> > Instantiate the ClientSocketHandle on the stack.  After you call Init() on
it,
> > just let it go out of scope.  You don't need to wait for the callback. 
Init()
> > will create a TCPConnectJob as long as we don't hit any limits (connections
> per
> > host or total tcp connections).  When ClientSocketHandle is destroyed, it'll
> > invoke Reset() which will, if the job succeeded synchronously, return the
> socket
> > back to the pool.  If it failed synchronously, it does nothing.  If it is
> > ERR_IO_PENDING, then it'll destroy the request, but not the TCPConnectJob
> (which
> > is late bound anyway, so it won't get cancelled, but will instead sit around
> in
> > the idle socket pool whenever it completes successfully).
> 
>

http://codereview.chromium.org/1585029/diff/55008/94011
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1585029/diff/55008/94011#newcode26
net/base/host_port_pair.h:26: // Equality test of contents. (Probably another
violation of style guide).
On 2010/04/30 02:02:27, jar wrote:
> The issue is that this is a struct, not a class, and hence is not supposed to
> have any methods other than accessors (I think).
> 
> On 2010/04/28 18:23:11, willchan wrote:
> > Your above comment on operator< is correct, but Equals() is recommended by
the
> > style guide (see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...).
> 
> 

Good point.  What do you think about moving Equals out to being a free function
that takes an |a| and |b| as parameters?

http://codereview.chromium.org/1585029/diff/84004/111004
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/84004/111004#newcode1
chrome/browser/net/preconnect.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
Copyright date.  Should be 2010.

http://codereview.chromium.org/1585029/diff/84004/111004#newcode29
chrome/browser/net/preconnect.cc:29: handle_.Reset();
I don't htink this is necessary.  IIRC, destroying the handle_ will call Reset()

http://codereview.chromium.org/1585029/diff/84004/111004#newcode33
chrome/browser/net/preconnect.cc:33: bool Preconnect::PreconnectOnUIThread(const
net::HostPortPair& hostport) {
Perhaps you should also DCHECK here that it's the UI thread.

http://codereview.chromium.org/1585029/diff/84004/111006
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/84004/111006#newcode21
chrome/browser/net/preconnect.h:21: class Preconnect : public
net::CompletionCallback,
I didn't comment on it earlier since I thought you would get rid of this object,
but I see your point about wanting to gather metrics when it completes.  In most
of the net/ code, we embed a CompletionCallbackImpl that calls OnIOComplete
rather than directly subclassing it.  Minor difference, but I suspect it'll be
more readable to more people who are familiar with the net code, so that's my
personal preference here.

http://codereview.chromium.org/1585029/diff/84004/111006#newcode27
chrome/browser/net/preconnect.h:27: static void PreconnectOnIOThread(const
net::HostPortPair& url);
s/url/hostport/

http://codereview.chromium.org/1585029/diff/84004/111006#newcode30
chrome/browser/net/preconnect.h:30:
Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
Please take this as a const scoped_refptr<net::TCPClientSocketPool>& instead. 
Saves on an unnecessary AddRef()/Release() pair.

[jar (doing other things), 2010-05-04 08:24:49]

More responses for WillChan and PKasting.

Please look to newest CL version to insert comments.

thx,

Jim

http://codereview.chromium.org/1585029/diff/55008/94007
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94007#newcode139
chrome/browser/net/dns_global.cc:139: const int kMinPreresolveSeconds(10);
I did think of this as a quiet bug fix (super minor perf boost).... but perhaps
you are right, and it might be making a notable difference if (for example) some
syn-flood detection was triggered by the old approach.  I'll put in a todo to
try to A/B test and see if this had any larger than expected impact.

On 2010/05/04 02:16:54, willchan wrote:
> On 2010/04/30 02:02:27, jar wrote:
> > On 2010/04/28 18:23:11, willchan wrote:
> > > Where are you getting this heuristic from?
> > 
> > I just tried to land some reasonable protective constants.
> > 
> > >  Are doing any A/B tests for these
> > > changes to stop doing DNS prefetching in certain cases?
> > 
> > I haven't A/B tested this, but it only reduces load.  Personal recent
testing
> > showed that the suggestions were coming in groups of two, only milliseconds
> > appart (on my machine).  It seemed that I should reduced the workload to
> > something reasonable by at least de-duping this here (and avoiding the
> numerous
> > calls, which would probably effectively de-dup this in the resolver, and
> > certainly waste resources getting to that point.
> > 
> 
> Do you think it makes a good perf impact?  If so, A/B testing would be nice. 
If
> you think it's too minor, then I'm fine just putting it in.

http://codereview.chromium.org/1585029/diff/55008/94007#newcode149
chrome/browser/net/dns_global.cc:149: const int kMaxSearchKeepaliveSeconds(30);
On 2010/05/04 02:16:54, willchan wrote:
> On 2010/04/30 02:02:27, jar wrote:
> > On 2010/04/28 18:23:11, willchan wrote:
> > > Where does this keep alive number come from?
> > 
> > This was another number I took a wild guess about.  Given that search URLs
> will
> > stay alive for some period of time (perhaps closer to 1-5 minutes), it
seemed
> > that I shouldn't try to pre-warm a connection more often than every 30
> seconds. 
> > I could probably use a longer period of time, but I just wanted to reduce
load
> > on the rest of the system by deduping these calls below some interval.
> 
> I think comments on this could be useful.

Done.

http://codereview.chromium.org/1585029/diff/55008/94008
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94008#newcode13
chrome/browser/net/preconnect.cc:13:
Preconnect::Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
On 2010/05/04 02:16:54, willchan wrote:
> On 2010/04/30 02:02:27, jar wrote:
>> ...
> You must be careful here.  Remember, this is the late binding API.
>  So, you will get the first available socket according to your priority.  It
> won't necessarily exactly match the socket that you're trying to prewarm.  If
> you need something like that, it might be better to add something explicitly
> into the ClientSocketHandle/ClientSocketPool API so that the ClientSocketPool
> itself can do the metric tracking.  This is what we do for NetLog tracing of
> backup connect jobs for the dropped SYNs.

I'm not groking the issue here, and will try to chat.  I didn't think there was
a distinct pool for high priority (disused or out-of-use or un-used) sockets, vs
low priority.

> 
> > 
> > I'm also a little wary that future optimizations may cause the
TCPConnectJob,
> > but maybe my fears of this interface are not justified.
> 

<hmmm... not sure how that typo happened> To finish what I was trying to say:

 I was fearful that someday the dropping of the ref might cause the would-be
connection prewarming to be abandoned prior to the arrival of the SYN's ACK. It
seemed safer and clearer to hold onto this ref until we had a response in hand
(a connection was REALLY opened).

http://codereview.chromium.org/1585029/diff/55008/94011
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1585029/diff/55008/94011#newcode26
net/base/host_port_pair.h:26: // Equality test of contents. (Probably another
violation of style guide).
On 2010/05/04 02:16:54, willchan wrote:
> On 2010/04/30 02:02:27, jar wrote:
> > The issue is that this is a struct, not a class, and hence is not supposed
to
> > have any methods other than accessors (I think).
> > 
> > On 2010/04/28 18:23:11, willchan wrote:
> > > Your above comment on operator< is correct, but Equals() is recommended by
> the
> > > style guide (see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...).
> > 
> > 
> 
> Good point.  What do you think about moving Equals out to being a free
function
> that takes an |a| and |b| as parameters?

I think moving out the Equals and outlining the compare would be the
Stylistically correct way to do this.  ...but... it would IMO also be worth
stepping up to full-class-status, and it might make the code a bit simpler.

In any case, I didn't want to do these changes in this CL, as it would probably
pull in changes to all users of this class... I just wanted to at least leave a
TODO (and I should really file a bug, and refer to it).

http://codereview.chromium.org/1585029/diff/84004/111006
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/84004/111006#newcode21
chrome/browser/net/preconnect.h:21: class Preconnect : public
net::CompletionCallback,
I'll ping you tomorrow, or talk to WTC to discuss this alternative.

On 2010/05/04 02:16:54, willchan wrote:
> I didn't comment on it earlier since I thought you would get rid of this
object,
> but I see your point about wanting to gather metrics when it completes.  In
most
> of the net/ code, we embed a CompletionCallbackImpl that calls OnIOComplete
> rather than directly subclassing it.  Minor difference, but I suspect it'll be
> more readable to more people who are familiar with the net code, so that's my
> personal preference here.

http://codereview.chromium.org/1585029/diff/84004/111006#newcode27
chrome/browser/net/preconnect.h:27: static void PreconnectOnIOThread(const
net::HostPortPair& url);
On 2010/05/04 02:16:54, willchan wrote:
> s/url/hostport/

Done.

http://codereview.chromium.org/1585029/diff/84004/111006#newcode30
chrome/browser/net/preconnect.h:30:
Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
On 2010/05/04 02:16:54, willchan wrote:
> Please take this as a const scoped_refptr<net::TCPClientSocketPool>& instead. 
> Saves on an unnecessary AddRef()/Release() pair.

Done.

http://codereview.chromium.org/1585029/diff/151002/146002
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1585029/diff/151002/146002#newcode574
chrome/browser/autocomplete/autocomplete_edit.cc:574:
UMA_HISTOGRAM_ENUMERATION("Autocomplete.MatchType", type,
I'll probably also record info on prewarmed connections, but that will mix in a
lot of other data (how long to create? got a new one vs recycled? ...).

I just wanted to get a better picture of how often each of these categories were
used.

On 2010/05/04 02:01:16, Peter Kasting wrote:
> Are you sure you want this here?  Observe() gets called a lot (multiple times
> per keystroke), and if you note a separate data point each time, you may skew
> your results.

http://codereview.chromium.org/1585029/diff/151002/146002#newcode577
chrome/browser/autocomplete/autocomplete_edit.cc:577: // These cases are for
default search service, specified by the user.
On 2010/05/04 02:01:16, Peter Kasting wrote:
> Nit: This comment doesn't seem to be accurate now that SEARCH_OTHER_ENGINE is
in
> the list.

Done. (Comments changed).

http://codereview.chromium.org/1585029/diff/151002/146002#newcode578
chrome/browser/autocomplete/autocomplete_edit.cc:578: case
AutocompleteMatch::SEARCH_WHAT_YOU_TYPED:
This may well end up being yet another know that I need to tweak and test to
optimize.

On 2010/05/04 02:01:16, Peter Kasting wrote:
> Of these four types, this is the lowest-quality signal -- you'll get this for
> all inputs that don't inline autocomplete and aren't a valid address on their
> own, which might be a lot.  Dunno whether you always want to prewarm for it.

[Peter Kasting, 2010-05-04 18:20:03]

On 2010/05/04 08:24:49, jar wrote:
> http://codereview.chromium.org/1585029/diff/151002/146002#newcode577
> chrome/browser/autocomplete/autocomplete_edit.cc:577: // These cases are for
> default search service, specified by the user.
> On 2010/05/04 02:01:16, Peter Kasting wrote:
> > Nit: This comment doesn't seem to be accurate now that SEARCH_OTHER_ENGINE
is
> in
> > the list.
> 
> Done. (Comments changed).

OK.  Your newest patch doesn't show any changes, though.

[jar (doing other things), 2010-05-04 18:36:39]

This (really) contains the one-line comment edit for pkasting.

http://codereview.chromium.org/1585029/diff/184002/172003
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1585029/diff/184002/172003#newcode582
chrome/browser/autocomplete/autocomplete_edit.cc:582: // engine (other than the
default).
@pkasting:  I have no idea why this didn't get pushed with my last upload....
but this is the comment change.

[Peter Kasting, 2010-05-04 18:50:20]

http://codereview.chromium.org/1585029/diff/184002/172003
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1585029/diff/184002/172003#newcode582
chrome/browser/autocomplete/autocomplete_edit.cc:582: // engine (other than the
default).
On 2010/05/04 18:36:39, jar wrote:
> @pkasting:  I have no idea why this didn't get pushed with my last upload....
> but this is the comment change.

Thanks!

Your comments might be clearer as:

// Matches using the user's default search engine.

and

// A match that uses a non-default search engine (e.g. for tab-to-search).

[jar (doing other things), 2010-05-04 19:47:37]

Changes per PKasting's comments.

http://codereview.chromium.org/1585029/diff/184002/172003
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/1585029/diff/184002/172003#newcode582
chrome/browser/autocomplete/autocomplete_edit.cc:582: // engine (other than the
default).
On 2010/05/04 18:50:20, Peter Kasting wrote:
> On 2010/05/04 18:36:39, jar wrote:
> > @pkasting:  I have no idea why this didn't get pushed with my last
upload....
> > but this is the comment change.
> 
> Thanks!
> 
> Your comments might be clearer as:
> 
> // Matches using the user's default search engine.
> 
> and
> 
> // A match that uses a non-default search engine (e.g. for tab-to-search).

Done.

[willchan no longer on Chromium, 2010-05-05 01:24:59]

I need to look at this again.  I'll do another pass tonight or tomorrow morning.

http://codereview.chromium.org/1585029/diff/55008/94008
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/55008/94008#newcode13
chrome/browser/net/preconnect.cc:13:
Preconnect::Preconnect(scoped_refptr<net::TCPClientSocketPool> pool,
On 2010/05/04 08:24:49, jar wrote:
> On 2010/05/04 02:16:54, willchan wrote:
> > On 2010/04/30 02:02:27, jar wrote:
> >> ...
> > You must be careful here.  Remember, this is the late binding API.
> >  So, you will get the first available socket according to your priority.  It
> > won't necessarily exactly match the socket that you're trying to prewarm. 
If
> > you need something like that, it might be better to add something explicitly
> > into the ClientSocketHandle/ClientSocketPool API so that the
ClientSocketPool
> > itself can do the metric tracking.  This is what we do for NetLog tracing of
> > backup connect jobs for the dropped SYNs.
> 
> I'm not groking the issue here, and will try to chat.  I didn't think there
was
> a distinct pool for high priority (disused or out-of-use or un-used) sockets,
vs
> low priority.

What I mean is that if let's say you start pre-warming a socket.  While the
connect is happening, someone else requests the socket.  This will kick off a
second socket connect.  There are now 2 requests (the pre-warming request and
the real request) and 2 jobs (the first one started by the pre-warming request
and the second one which is real).  The first job (kicked off by the pre-warming
request) finishes first.  It gets handed to the 2nd request (the real request). 
The 2nd job finishes afterward.  It gets handed to the 1st request (the
pre-warming request).  In this fashion, the pre-warming request may not get the
socket that it initiated the connect() for.

> 
> > 
> > > 
> > > I'm also a little wary that future optimizations may cause the
> TCPConnectJob,
> > > but maybe my fears of this interface are not justified.
> > 
> 
> <hmmm... not sure how that typo happened> To finish what I was trying to say:
> 
>  I was fearful that someday the dropping of the ref might cause the would-be
> connection prewarming to be abandoned prior to the arrival of the SYN's ACK.
It
> seemed safer and clearer to hold onto this ref until we had a response in hand
> (a connection was REALLY opened).


Due to socket late binding, the jobs are almost completely disconnected from the
requests.  The only ways in which they can influence each other is:
(1) starting a request will start up a job if there are fewer jobs than
requests.
(2) canceling a request will cancel a job if there are fewer jobs than requests
+ 2.

Of course, this is not exposed in the API at all and is subject to change, so
keeping your ClientSocketHandle alive will guarantee it finishes to completion,
at the cost of a minor increase in code complexity.

http://codereview.chromium.org/1585029/diff/55008/94011
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/1585029/diff/55008/94011#newcode26
net/base/host_port_pair.h:26: // Equality test of contents. (Probably another
violation of style guide).
On 2010/05/04 08:24:49, jar wrote:
> On 2010/05/04 02:16:54, willchan wrote:
> > On 2010/04/30 02:02:27, jar wrote:
> > > The issue is that this is a struct, not a class, and hence is not supposed
> to
> > > have any methods other than accessors (I think).
> > > 
> > > On 2010/04/28 18:23:11, willchan wrote:
> > > > Your above comment on operator< is correct, but Equals() is recommended
by
> > the
> > > > style guide (see
> > > >
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...).
> > > 
> > > 
> > 
> > Good point.  What do you think about moving Equals out to being a free
> function
> > that takes an |a| and |b| as parameters?
> 
> I think moving out the Equals and outlining the compare would be the
> Stylistically correct way to do this.  ...but... it would IMO also be worth
> stepping up to full-class-status, and it might make the code a bit simpler.

I think you and I have a minor stylistic disagreement here.  I like keeping it
as a struct since it makes it obvious that it's just pure data.  I'd lean
towards making the existing member methods just be free functions.  That's just
my stylistic bias.  You admitted to me before you have a bias towards always
using classes.  I think therein lies our conflict.  I don't think there's a
point discussing it, but if we want to reach a resolution, I suggest asking wtc
or eroman for their stylistic pref and going with the majority here.

> 
> In any case, I didn't want to do these changes in this CL, as it would
probably
> pull in changes to all users of this class... I just wanted to at least leave
a
> TODO (and I should really file a bug, and refer to it).

Agreed.

http://codereview.chromium.org/1585029/diff/84004/111006
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/84004/111006#newcode21
chrome/browser/net/preconnect.h:21: class Preconnect : public
net::CompletionCallback,
On 2010/05/04 08:24:49, jar wrote:
> I'll ping you tomorrow, or talk to WTC to discuss this alternative.
> 
> On 2010/05/04 02:16:54, willchan wrote:
> > I didn't comment on it earlier since I thought you would get rid of this
> object,
> > but I see your point about wanting to gather metrics when it completes.  In
> most
> > of the net/ code, we embed a CompletionCallbackImpl that calls OnIOComplete
> > rather than directly subclassing it.  Minor difference, but I suspect it'll
be
> > more readable to more people who are familiar with the net code, so that's
my
> > personal preference here.
> 
> 

Did you discuss this with wtc?

[jar (doing other things), 2010-05-05 01:49:50]

On Tue, May 4, 2010 at 6:24 PM, <[email protected]> wrote:

>
> What I mean is that if let's say you start pre-warming a socket.  While
> the connect is happening, someone else requests the socket.  This will
> kick off a second socket connect.  There are now 2 requests (the
> pre-warming request and the real request) and 2 jobs (the first one
> started by the pre-warming request and the second one which is real).
> The first job (kicked off by the pre-warming request) finishes first.
> It gets handed to the 2nd request (the real request).  The 2nd job
> finishes afterward.  It gets handed to the 1st request (the pre-warming
> request).  In this fashion, the pre-warming request may not get the
> socket that it initiated the connect() for.
>
>
>
Hmm... I don't see the above sequence as a problem at all.  There can be
cases where two socket requests are outstanding.  If the "real" one gets
assigned first, then I say great. :-).  If the fake (preconnect) gets
assigned a socket first, then it would immediately abandon (reset) it, and
the socket would then be assigned to the "real" request... which is again,
just great.

Bottom line: I don't see a problem.

I think moving out the Equals and outlining the compare would be the
>> Stylistically correct way to do this.  ...but... it would IMO also be
>>
> worth
>
>> stepping up to full-class-status, and it might make the code a bit
>>
> simpler.
>
> I think you and I have a minor stylistic disagreement here.  I like
> keeping it as a struct since it makes it obvious that it's just pure
> data.  I'd lean towards making the existing member methods just be free
> functions.  That's just my stylistic bias.  You admitted to me before
> you have a bias towards always using classes.  I think therein lies our
> conflict.  I don't think there's a point discussing it, but if we want
> to reach a resolution, I suggest asking wtc or eroman for their
> stylistic pref and going with the majority here.
>
>
>
It is No Big Deal to me. ...and I won't be upset either way.... but I would
note that the style guide seems to be pretty clear [emphasis/color/bolding
mine]:


structs  should be used for passive objects that carry data, ..., but lack *any
functionality* other than access/setting the data members. ... If more
functionality is required, a class is more appropriate. *If in doubt, make
it a **class**.*



The fact that you suggest there is a stylist question means there is
"doubt."  Most methods can be turned into global functions (which is what
you'd like to do with the methods on this class, for comparison and for
sorting).  Using methods also seems slightly nicer for STL interactions, and
avoids name-space issues.  The *only* downside that I know of is that you
have the extra parens in a getter (but you change 3 characters " = " into
two parens in the and perchance a "set_" prefix in the setter... but you
could argue that "=" is more readable).

Again, it is no big deal... and I wasn't rushing to change it (and all the
code that depends on it).  My historical religious experience has shown time
and again that it was very convenient to set breakpoints on accessors, and
since the compiler optimizes them into identical code (to struct accesses),
my religion given me (I think) the best of both worlds. (both performance
and debugability).

[willchan no longer on Chromium, 2010-05-05 18:29:10]

On 2010/05/05 01:49:50, jar wrote:
> On Tue, May 4, 2010 at 6:24 PM, <mailto:[email protected]> wrote:
> 
> >
> > What I mean is that if let's say you start pre-warming a socket.  While
> > the connect is happening, someone else requests the socket.  This will
> > kick off a second socket connect.  There are now 2 requests (the
> > pre-warming request and the real request) and 2 jobs (the first one
> > started by the pre-warming request and the second one which is real).
> > The first job (kicked off by the pre-warming request) finishes first.
> > It gets handed to the 2nd request (the real request).  The 2nd job
> > finishes afterward.  It gets handed to the 1st request (the pre-warming
> > request).  In this fashion, the pre-warming request may not get the
> > socket that it initiated the connect() for.
> >
> >
> >
> Hmm... I don't see the above sequence as a problem at all.  There can be
> cases where two socket requests are outstanding.  If the "real" one gets
> assigned first, then I say great. :-).  If the fake (preconnect) gets
> assigned a socket first, then it would immediately abandon (reset) it, and
> the socket would then be assigned to the "real" request... which is again,
> just great.
> 
> Bottom line: I don't see a problem.

Ok, as long as you don't see a problem that's fine.  You had just mentioned that
you wanted to record metrics at this point, so if it's fine that you don't know
what socket you'll get at this point for your metrics recording, then it sounds
good to me.  I just wanted to make sure you were aware :)

> 
> I think moving out the Equals and outlining the compare would be the
> >> Stylistically correct way to do this.  ...but... it would IMO also be
> >>
> > worth
> >
> >> stepping up to full-class-status, and it might make the code a bit
> >>
> > simpler.
> >
> > I think you and I have a minor stylistic disagreement here.  I like
> > keeping it as a struct since it makes it obvious that it's just pure
> > data.  I'd lean towards making the existing member methods just be free
> > functions.  That's just my stylistic bias.  You admitted to me before
> > you have a bias towards always using classes.  I think therein lies our
> > conflict.  I don't think there's a point discussing it, but if we want
> > to reach a resolution, I suggest asking wtc or eroman for their
> > stylistic pref and going with the majority here.
> >
> >
> >
> It is No Big Deal to me. ...and I won't be upset either way.... but I would
> note that the style guide seems to be pretty clear [emphasis/color/bolding
> mine]:
> 
> 
> structs  should be used for passive objects that carry data, ..., but lack
*any
> functionality* other than access/setting the data members. ... If more
> functionality is required, a class is more appropriate. *If in doubt, make
> it a **class**.*
> 
> 
> 
> The fact that you suggest there is a stylist question means there is
> "doubt."  Most methods can be turned into global functions (which is what
> you'd like to do with the methods on this class, for comparison and for
> sorting).  Using methods also seems slightly nicer for STL interactions, and
> avoids name-space issues.  The *only* downside that I know of is that you
> have the extra parens in a getter (but you change 3 characters " = " into
> two parens in the and perchance a "set_" prefix in the setter... but you
> could argue that "=" is more readable).
> 
> Again, it is no big deal... and I wasn't rushing to change it (and all the
> code that depends on it).  My historical religious experience has shown time
> and again that it was very convenient to set breakpoints on accessors, and
> since the compiler optimizes them into identical code (to struct accesses),
> my religion given me (I think) the best of both worlds. (both performance
> and debugability).
>

[willchan no longer on Chromium, 2010-05-05 19:04:41]

Apparently there are some lint errors, I haven't looked at them to see if
they're valid, but you should look if you haven't.

I have no major holdups.  I've raised the concerns for things you should look
into, but if you're fine with them, then I am too.

LGTM.

http://codereview.chromium.org/1585029/diff/204001/172038
File chrome/browser/net/dns_master.h (right):

http://codereview.chromium.org/1585029/diff/204001/172038#newcode29
chrome/browser/net/dns_master.h:29: using base::TimeDelta;
This is against the style guide.  You don't have to fix it here, but we should
fix it at some point.

http://codereview.chromium.org/1585029/diff/204001/172038#newcode167
chrome/browser/net/dns_master.h:167: bool WasFound(const net::HostPortPair&
hostport) {
Can this be const?

Also, it looks like you're doing two lookups here.  You should only have to do
one.  This is existing code, so it's fine to leave as is.

http://codereview.chromium.org/1585029/diff/204001/172029
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/204001/172029#newcode21
chrome/browser/net/preconnect.cc:21: AddRef();  // Init() will hold a reference
until the callback.
I just realized another thing about holding onto the handle.  What's your
desired behavior here when prewarming?  To the ClientSocketPool, when you
initialize the prewarming handle, it'll start a TCPConnectJob.  When a real
request comes in, it'll start a 2nd TCPConnectJob.  Do you actually want a 2nd
TCPConnectJob?  Are we going to end up firing off more jobs than we actually
want?  Remember that there will be at least as many jobs as there are
handles/requests alive for that HostPortPair, subject to the 6 connection per
host limit.  I just want to verify with you that that's what you want here.

http://codereview.chromium.org/1585029/diff/204001/172029#newcode29
chrome/browser/net/preconnect.cc:29: if (handle_.is_initialized())
Did you answer this earlier about why you have this here?  I think that the
destructor for handle_ should reset it automatically.

http://codereview.chromium.org/1585029/diff/204001/172031
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/204001/172031#newcode21
chrome/browser/net/preconnect.h:21: class Preconnect : public
net::CompletionCallback,
You still haven't changed this.  My preference is to compose it, but I won't
block the change.

http://codereview.chromium.org/1585029/diff/204001/172031#newcode27
chrome/browser/net/preconnect.h:27: static void PreconnectOnIOThread(const
net::HostPortPair& url);
s/url/hostport/

[Mike Belshe, 2010-05-12 21:21:52]

http://codereview.chromium.org/1585029/diff/204001/172029
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/204001/172029#newcode17
chrome/browser/net/preconnect.cc:17: net::TCPSocketParams params(hostport.host,
hostport.port, net::LOW,
We should think a little about the socket pools here; when you expand this code
out to support other pool types, it will get interesting.  (See
HttpNetworkTransaction::Start())

http://codereview.chromium.org/1585029/diff/204001/172029#newcode20
chrome/browser/net/preconnect.cc:20: // TODO(jar): This does not handle proxies
currently.
I guess this TODO needs to be implemented before this can be turned on?

[jar (doing other things), 2010-05-15 07:03:42]

Will,

I've changed the preconnect.* files to release the handle_ asap.  Please review
these changes carefully.

There are also several questions in the comment responses.

Thanks!

http://codereview.chromium.org/1585029/diff/204001/172038
File chrome/browser/net/dns_master.h (right):

http://codereview.chromium.org/1585029/diff/204001/172038#newcode29
chrome/browser/net/dns_master.h:29: using base::TimeDelta;
On 2010/05/05 19:04:42, willchan wrote:
> This is against the style guide.  You don't have to fix it here, but we should
> fix it at some point.

Done. (Fixed)

http://codereview.chromium.org/1585029/diff/204001/172038#newcode167
chrome/browser/net/dns_master.h:167: bool WasFound(const net::HostPortPair&
hostport) {
On 2010/05/05 19:04:42, willchan wrote:
> Can this be const?
> 
> Also, it looks like you're doing two lookups here.  You should only have to do
> one.  This is existing code, so it's fine to leave as is.

Done.

http://codereview.chromium.org/1585029/diff/204001/172029
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/204001/172029#newcode17
chrome/browser/net/preconnect.cc:17: net::TCPSocketParams params(hostport.host,
hostport.port, net::LOW,
The interface has gone back and forth about whether I need to specify a scheme
(http vs https).  

@WillChan: What is the current status of this interface? Is the distinction
needed?  ...or is that all pending support for https preconnection (with
backaway when client auth is requested... and back-away when UI notification of
an invalid cert chain detection).

On 2010/05/12 21:21:52, Mike Belshe wrote:
> We should think a little about the socket pools here; when you expand this
code
> out to support other pool types, it will get interesting.  (See
> HttpNetworkTransaction::Start())

http://codereview.chromium.org/1585029/diff/204001/172029#newcode20
chrome/browser/net/preconnect.cc:20: // TODO(jar): This does not handle proxies
currently.
I need to discuss further with WillChan.  At one point he suggested that some
additional work was needed... and then I think suggested that it might just
work... but perhaps I should not be pre-connecting at all in a proxy (limited
connection) scenario.

On 2010/05/12 21:21:52, Mike Belshe wrote:
> I guess this TODO needs to be implemented before this can be turned on?

http://codereview.chromium.org/1585029/diff/204001/172029#newcode21
chrome/browser/net/preconnect.cc:21: AddRef();  // Init() will hold a reference
until the callback.
I now drop the handle asap (right after calling init()).

On 2010/05/05 19:04:42, willchan wrote:
> I just realized another thing about holding onto the handle.  What's your
> desired behavior here when prewarming?  To the ClientSocketPool, when you
> initialize the prewarming handle, it'll start a TCPConnectJob.  When a real
> request comes in, it'll start a 2nd TCPConnectJob.  Do you actually want a 2nd
> TCPConnectJob?  Are we going to end up firing off more jobs than we actually
> want?  Remember that there will be at least as many jobs as there are
> handles/requests alive for that HostPortPair, subject to the 6 connection per
> host limit.  I just want to verify with you that that's what you want here.

http://codereview.chromium.org/1585029/diff/204001/172029#newcode29
chrome/browser/net/preconnect.cc:29: if (handle_.is_initialized())
On 2010/05/05 19:04:42, willchan wrote:
> Did you answer this earlier about why you have this here?  I think that the
> destructor for handle_ should reset it automatically.

Done.

http://codereview.chromium.org/1585029/diff/204001/172031
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/204001/172031#newcode21
chrome/browser/net/preconnect.h:21: class Preconnect : public
net::CompletionCallback,
I've transitioned to using a single instance of the class for the callback
parameter, and I don't use the class otherwise.

On 2010/05/05 19:04:42, willchan wrote:
> You still haven't changed this.  My preference is to compose it, but I won't
> block the change.

http://codereview.chromium.org/1585029/diff/204001/172031#newcode27
chrome/browser/net/preconnect.h:27: static void PreconnectOnIOThread(const
net::HostPortPair& url);
On 2010/05/05 19:04:42, willchan wrote:
> s/url/hostport/

Done.

[willchan no longer on Chromium, 2010-05-17 19:14:14]

http://codereview.chromium.org/1585029/diff/204001/172029
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/204001/172029#newcode17
chrome/browser/net/preconnect.cc:17: net::TCPSocketParams params(hostport.host,
hostport.port, net::LOW,
On 2010/05/15 07:03:42, jar wrote:
> The interface has gone back and forth about whether I need to specify a scheme
> (http vs https).  
> 
> @WillChan: What is the current status of this interface? Is the distinction
> needed?  ...or is that all pending support for https preconnection (with
> backaway when client auth is requested... and back-away when UI notification
of
> an invalid cert chain detection).

The http vs https distinction is unfortunately needed, since apparently some
servers serve both http and https over the same port.

> 
> On 2010/05/12 21:21:52, Mike Belshe wrote:
> > We should think a little about the socket pools here; when you expand this
> code
> > out to support other pool types, it will get interesting.  (See
> > HttpNetworkTransaction::Start())
> 
>

http://codereview.chromium.org/1585029/diff/204001/172029#newcode20
chrome/browser/net/preconnect.cc:20: // TODO(jar): This does not handle proxies
currently.
On 2010/05/15 07:03:42, jar wrote:
> I need to discuss further with WillChan.  At one point he suggested that some
> additional work was needed... and then I think suggested that it might just
> work... but perhaps I should not be pre-connecting at all in a proxy (limited
> connection) scenario.

Further work is needed.  And yes, it's possible you don't want to pre-connect at
all in a proxy since you will be limited to 15 connections anyways.

> 
> On 2010/05/12 21:21:52, Mike Belshe wrote:
> > I guess this TODO needs to be implemented before this can be turned on? 
> 
>

http://codereview.chromium.org/1585029/diff/260001/247016#newcode18
chrome/browser/net/preconnect.cc:18: GURL(), false);
Why is this in a scoped_refptr?  Does it need to be ref counted?  I'd just
allocate it and leak it.  a scoped_refptr<> is not a POD type, so the global
objects style guide prohibition applies to it too.

http://codereview.chromium.org/1585029/diff/260001/247016#newcode40
chrome/browser/net/preconnect.cc:40: ChromeThread::PostTask(
You should provide an error string ("Not running on IO thread!" or something),
since the LOG(ERROR) won't show much useful without it.

[jar (doing other things), 2010-05-17 23:11:53]

Two changes made per Will's suggestion.

If I get an LGTM, I'll land its under a flag, and then restructure to
distinguish SSL vs non-SSL connections.  As it currently stands, it supports
HTTP, and would misunderstand an SSL connection to be an HTTP connection, and
preconnect over HTTP (which won't help a future HTTPS connection).

http://codereview.chromium.org/1585029/diff/260001/247016
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/1585029/diff/260001/247016#newcode18
chrome/browser/net/preconnect.cc:18: scoped_refptr<Preconnect>
Preconnect::callback_instance_;
On 2010/05/17 19:14:14, willchan wrote:
> Why is this in a scoped_refptr?  Does it need to be ref counted?  I'd just
> allocate it and leak it.  a scoped_refptr<> is not a POD type, so the global
> objects style guide prohibition applies to it too.

Done.

http://codereview.chromium.org/1585029/diff/260001/247016#newcode40
chrome/browser/net/preconnect.cc:40: LOG(DFATAL);
On 2010/05/17 19:14:14, willchan wrote:
> You should provide an error string ("Not running on IO thread!" or something),
> since the LOG(ERROR) won't show much useful without it.

Done.

[willchan no longer on Chromium, 2010-05-17 23:50:08]

LGTM

http://codereview.chromium.org/1585029/diff/275001/258017
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/275001/258017#newcode13
chrome/browser/net/preconnect.h:13: #include "net/base/host_port_pair.h"
I think you can forward declare net::HostPortPair and don't need the rest of the
net includes below this line.  Might need to move to the .cc.

[jar (doing other things), 2010-05-18 00:52:57]

Comment on Will's suggestion.

http://codereview.chromium.org/1585029/diff/275001/258017
File chrome/browser/net/preconnect.h (right):

http://codereview.chromium.org/1585029/diff/275001/258017#newcode13
chrome/browser/net/preconnect.h:13: #include "net/base/host_port_pair.h"
I'm going to migrate to using a canonicalized GURL (scheme, host, port only)
instead of host_port_pair, so I'll handle this stuff in the next patch.

Jim

On 2010/05/17 23:50:08, willchan wrote:
> I think you can forward declare net::HostPortPair and don't need the rest of
the
> net includes below this line.  Might need to move to the .cc.