* Move the global "DnsResolutionObserver" code depended on by DNS prefetcher,...

* Move the global "DnsResolutionObserver" code depended on by DNS prefetcher, into HostResolver.  This has the advantage that consumers of DNS no longer have to remember to call "DidFinishDnsResolutionWithStatus()" followed by "DidStartDnsResolution()" in order for the prefetcher to observe the resolution. Instead it just happens automatically, and subscribers register via HostResolver::AddObserver() on a particular resolver instance.

* To accomodate the prefetcher's observer, HostResolver::Resolve() needs an additional "referrer" parameter. This is slightly awkward since "referrer" has nothing to do with the actual resolve request. To simplify plumbing through this and other optional parameters, Resolve() was changed to take a "RequestInfo&" parameter in place of say {hostname, port, flags}.

* Added an option to HostResolver::Resolve() for disallowing cached responses (RequestInfo::allow_cached_response). This will be used when you refresh a page, to bypass the host cache. The code to do this has been added to HttpNetworkTransaction, but is commented out pending an appropriate unit-test to verify it.

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

[eroman, 2009-06-14 09:23:47]

jar: please review.

willchan: just FYI, since i made slight changes to client socket pool interface.

[jar (doing other things), 2009-06-14 15:30:05]

Generally the change looks quite good.  I had a few questions and comments below
that I'd like you to consider.

I'll be busied up later this morning and into the afternoon, but I should be
able to look at the CL again later today.

Thanks again for spotting this regression!  It should be a big performance loss
that this CL will restore.

http://codereview.chromium.org/125107/diff/1043/1062
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/125107/diff/1043/1062#newcode146
Line 146: virtual void OnStartResolution(int requet_id,
nit: requet_id -> request_id

http://codereview.chromium.org/125107/diff/1043/1062#newcode205
Line 205: // FIXME: If the resolve request is cancelled, then
Should be a TODO()... unless you're still writing for this CL.

Can you give an example where a request is cancelled?  Is it when a navigation
is cancelled... and the DNS resolution happens to be what is currently
happening?

http://codereview.chromium.org/125107/diff/1043/1062#newcode413
Line 413: // host resolver lives).
Please add TODO()...

http://codereview.chromium.org/125107/diff/1043/1061
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/125107/diff/1043/1061#newcode39
Line 39: net::HostResolver::RequestInfo info(hostname_, 80);
naming nit: In this file, |info| is generally used for DnsHostInfo instances...
and it is used a lot.

Better name might be |resolver_info|

http://codereview.chromium.org/125107/diff/1043/1061#newcode43
Line 43: info.referrer = GURL("dns-prefetcher://request/");
Although structs can be used, classes with accessor methods are more debuggable,
and equally efficient.


GURLs are about as easy to construct as a string... but it seems a waste to do
this when all we really want to signal is that this is a prefetch resolution. 
I'd suggest a boolean flag in the RequestInfo class that defaults to false, but
is only set here. Although not a dramatic efficiency issued, this would probably
be clearer and more efficient.

http://codereview.chromium.org/125107/diff/1043/1051
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/125107/diff/1043/1051#newcode451
Line 451: callback, addresses, info.port, request_id, info.referrer);
It is tempting to continue to push the |info| struct or class into this
interface.  Did you stop here to reduce complexity of the CL?

http://codereview.chromium.org/125107/diff/1043/1051#newcode470
Line 470: // http://crbug.com/9598
You may want to update that bug.  IMO, the de-duping of active requests should
greatly reduce the maximum count, and may make this bug a non-issue.

http://codereview.chromium.org/125107/diff/1043/1050
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/125107/diff/1043/1050#newcode650
Line 650: reinterpret_cast<net::CompletionCallback*> (1);
I'd suggest defining a NeverCalledCallback() that does an an creates a failing
test.  Using a bogus pointer would lead to a crash in the test (if it was
incorrectly called).

http://codereview.chromium.org/125107/diff/1043/1050#newcode729
Line 729: was_resolved == other.was_resolved &&
nit: 4 space indent

http://codereview.chromium.org/125107/diff/1043/1046
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/125107/diff/1043/1046#newcode343
Line 343: info.referrer = GURL(); // TODO(eroman): What to put?
If we don't plumb a calling API that includes a referrer, then we just don't get
the subresource pre-resolution optimization.  In edge cases like this, there is
no problem. If any of these cases prove significant, we can go back and plumb an
API to facilitate the filling in of the GURL.  You might want to comment to that
effect rather than leaving a TODO here.  

I'd suggest replacing the whole line (TODO and assignment) with something like:
// info has no known referrer at this point.

http://codereview.chromium.org/125107/diff/1043/1046#newcode837
Line 837: info.referrer = GURL();  // TODO(eroman): What to put?
Probably delete TODO and/or line.

http://codereview.chromium.org/125107/diff/1043/1044
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/125107/diff/1043/1044#newcode577
Line 577: // with the page triggered them. It doesn't impact the actual
addresses
nit typo: page triggered --> page that triggered

http://codereview.chromium.org/125107/diff/1043/1044#newcode585
Line 585: // If the user is refreshing the page, bypass the host cache.
I really suspect that the critical case is a negative resolution cache. I'm also
not sure that plumbing of the use of the refresh key is that plausible.  As a
result, I'd argue that when a navigation resolution gets a cached negative
result, there should be (in addition to the rapid negative response) a
background job to re-resolve the name.  I think such is more passive, and more
likely to work cosistenty.

[eroman, 2009-06-14 23:22:39]

http://codereview.chromium.org/125107/diff/1043/1062
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/125107/diff/1043/1062#newcode146
Line 146: virtual void OnStartResolution(int requet_id,
On 2009/06/14 15:30:06, jar wrote:
> nit: requet_id -> request_id

Done.

http://codereview.chromium.org/125107/diff/1043/1062#newcode205
Line 205: // FIXME: If the resolve request is cancelled, then
On 2009/06/14 15:30:06, jar wrote:
> Should be a TODO()... unless you're still writing for this CL.

Done.

> Can you give an example where a request is cancelled?  Is it when a navigation
> is cancelled... and the DNS resolution happens to be what is currently
> happening?

Right. So this was a pre-existing problem I noticed when reading the code. I
left it as a TODO for tackling later.

The problem was if you closed a tab while a host resolution was in progress (or
cancelled the request in any other number of other ways), the
HttpNetworkTransaction gets deleted. This cancels any outstanding host resolves,
and hence the matching "DidFinishDnsResolutionWithStatus()" will never be
called. Consequently the entries added to resolutions[] during
"DidStartDnsResolution()" do not get removed if the request is cancelled.

The good news is, we are better equipped to deal with that edge case now. Since
observers have become an internal concept of HostResolver, we know when the
request has been cancelled, and could export an OnCancelledResolution() event
for observers. Alternately we could do a lazy checks to see if resolutions ever
grows too big (say > 100), and then just blow it away.

http://codereview.chromium.org/125107/diff/1043/1062#newcode413
Line 413: // host resolver lives).
On 2009/06/14 15:30:06, jar wrote:
> Please add TODO()...

Done.

Even though we do the initialization on UI thread, there is no race since the IO
thread hasn't started running requests yet... but it is weird and should be
fixed.

http://codereview.chromium.org/125107/diff/1043/1061
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/125107/diff/1043/1061#newcode39
Line 39: net::HostResolver::RequestInfo info(hostname_, 80);
On 2009/06/14 15:30:06, jar wrote:
> naming nit: In this file, |info| is generally used for DnsHostInfo
instances...
> and it is used a lot.
> 
> Better name might be |resolver_info|

Done.
Renamed to "resolve_info".

http://codereview.chromium.org/125107/diff/1043/1061#newcode43
Line 43: info.referrer = GURL("dns-prefetcher://request/");
On 2009/06/14 15:30:06, jar wrote:
> Although structs can be used, classes with accessor methods are more
debuggable,
> and equally efficient.
> 
> 
> GURLs are about as easy to construct as a string... but it seems a waste to do
> this when all we really want to signal is that this is a prefetch resolution. 
> I'd suggest a boolean flag in the RequestInfo class that defaults to false,
but
> is only set here. Although not a dramatic efficiency issued, this would
probably
> be clearer and more efficient.

Done.

Changed to:

// Make a note that this is a speculative resolve request. This allows us
// to separate it from real navigations in the observer's callback, and
// lets the HostResolver know it can de-prioritize it.
resolve_request.set_is_speculative(true);

http://codereview.chromium.org/125107/diff/1043/1051
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/125107/diff/1043/1051#newcode451
Line 451: callback, addresses, info.port, request_id, info.referrer);
On 2009/06/14 15:30:06, jar wrote:
> It is tempting to continue to push the |info| struct or class into this
> interface.  Did you stop here to reduce complexity of the CL?

Done.

(I had not done it to avoid copying |hostname| string, but it definitely makes
for cleaner interfaces to simply push the RequestInfo into the Request. I have
also now updated the Observer callback to take a "const RequestInfo&" parameter
in place of {hostname, referrer}).

http://codereview.chromium.org/125107/diff/1043/1051#newcode470
Line 470: // http://crbug.com/9598
On 2009/06/14 15:30:06, jar wrote:
> You may want to update that bug.  IMO, the de-duping of active requests should
> greatly reduce the maximum count, and may make this bug a non-issue.

I still want that bug done.

In addition, now that we know which requests are issued by the DNS prefetcher,
we can do some fancier global scheduling. For example, if we already have a
number of non-speculative jobs in progress, we can outright drop the DNS
prefetcher's request. Or we can queue it with low priority. (The throttling we
currently do in the prefetcher doesn't take into account the current load for
non-speculative requests.)

http://codereview.chromium.org/125107/diff/1043/1050
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/125107/diff/1043/1050#newcode650
Line 650: reinterpret_cast<net::CompletionCallback*> (1);
On 2009/06/14 15:30:06, jar wrote:
> I'd suggest defining a NeverCalledCallback() that does an an creates a failing
> test.  Using a bogus pointer would lead to a crash in the test (if it was
> incorrectly called).

I am fine if the test crashes if it uses junk_callback, since that means it
failed. That lets me avoid writing more boiler plate to bind a valid callback
:). If you are unconvinced I can spew out some more code.

http://codereview.chromium.org/125107/diff/1043/1050#newcode729
Line 729: was_resolved == other.was_resolved &&
On 2009/06/14 15:30:06, jar wrote:
> nit: 4 space indent

Done.

(I lined it up with the previous line.)

http://codereview.chromium.org/125107/diff/1043/1046
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/125107/diff/1043/1046#newcode343
Line 343: info.referrer = GURL(); // TODO(eroman): What to put?
On 2009/06/14 15:30:06, jar wrote:
> If we don't plumb a calling API that includes a referrer, then we just don't
get
> the subresource pre-resolution optimization.  In edge cases like this, there
is
> no problem. If any of these cases prove significant, we can go back and plumb
an
> API to facilitate the filling in of the GURL.  You might want to comment to
that
> effect rather than leaving a TODO here.  
> 
> I'd suggest replacing the whole line (TODO and assignment) with something
like:
> // info has no known referrer at this point.
> 

Done.

http://codereview.chromium.org/125107/diff/1043/1044
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/125107/diff/1043/1044#newcode577
Line 577: // with the page triggered them. It doesn't impact the actual
addresses
On 2009/06/14 15:30:06, jar wrote:
> nit typo: page triggered --> page that triggered

Done.

http://codereview.chromium.org/125107/diff/1043/1044#newcode585
Line 585: // If the user is refreshing the page, bypass the host cache.
On 2009/06/14 15:30:06, jar wrote:
> I really suspect that the critical case is a negative resolution cache. I'm
also
> not sure that plumbing of the use of the refresh key is that plausible.  As a
> result, I'd argue that when a navigation resolution gets a cached negative
> result, there should be (in addition to the rapid negative response) a
> background job to re-resolve the name.  I think such is more passive, and more
> likely to work cosistenty.

It sounds like you are talking about something different. This is not about
negative cache entries, but about when user hits F5 to "refresh" a page.

For such a load, we already bypass the HTTP disk cache. For symmetry and
correctness, we should similarly bypass the host cache.

The concept is the same as with HTTP cache: it is possible we have a positive
valid cached resource, but this resource differs from the state of the network
resource. (For example we might have a host cache entry which maps to an
existing server, but this is the WRONG server).

Therefore if the user asks to bypass the cache, we should respect it. (nothing
more annoying then hitting refresh and still seeing what you know to be the old
version of something!).

Either way, that change is not actually part of this CL (note that it is
commented out).

[jar (doing other things), 2009-06-15 04:25:45]

LGTM++++++

I'm really psyched to see this land.  This bug is definitely impacting
statistics we're gathering, and negatively impacting the user.

p.s., See side comment below.

http://codereview.chromium.org/125107/diff/1043/1051
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/125107/diff/1043/1051#newcode470
Line 470: // http://crbug.com/9598
On 2009/06/14 23:22:39, eroman wrote:
> On 2009/06/14 15:30:06, jar wrote:
> > You may want to update that bug.  IMO, the de-duping of active requests
should
> > greatly reduce the maximum count, and may make this bug a non-issue.
> 
> I still want that bug done.
> 
> In addition, now that we know which requests are issued by the DNS prefetcher,
> we can do some fancier global scheduling. For example, if we already have a
> number of non-speculative jobs in progress, we can outright drop the DNS
> prefetcher's request. Or we can queue it with low priority. (The throttling we
> currently do in the prefetcher doesn't take into account the current load for
> non-speculative requests.)

It turns out that the total number of "speculative" requests is very bounded
(currently 8 max by default).  The only reason for the number of outstanding
resolution requests to grow large is because users are really navigating to all
these (now distinct) domains!?!  In that situation, removing a fixed constant
(the speculative ones?) would have little impact.

Also note that IF you decide to try to "discard" speculative resoltion, the
correct systematic thing to do is to delay them as long as you need to in a
queue, rather than "discard them."  A delay will correctly put back-pressure on
the pre-resolution system to discard its internal queue.  By delaying them, all
you end up doing is "taking a long time to resolve" and at the same time, you
throttle them implicitly.  If you really "dropped" the request, then there would
be a question of what you return to the pre-resolution system (found? not
found?), and you'd end up getting more requests that you'd end up having to give
equally non-answers to. Delaying is the way to go.... but as I said... I really
think the de-duping of contemporaneous requests effectively solved the bug.

The *only* way I can imagine the bug continuing to be present is IF there are
MANY tabs, and each tab is autoloading... an each autoload causes its own set of
navigational resolutions (to new domains.. which are not cached??).  I'm very
hard pressed to believe that is a real-world scenario (beyond some internet
wide, auto-crawling JS system).