Add an additional per-request DNS cache when executing FindProxyForURL() from...

Add an additional per-request DNS cache when executing FindProxyForURL() from a PAC script.

This mini-cache is more aggressive in caching negative resolutions, to avoid performance problems with PAC scripts that do lots of serial DNS resolves.

This is necessary now that we no longer globally cache DNS resolve failures to avoid performance regressions.

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

[wtc, 2010-06-22 02:24:16]

LGTM.  Good patch, and thank you for explaining it to me in
person.

http://codereview.chromium.org/2833021/diff/13001/14002
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/2833021/diff/13001/14002#newcode104
net/proxy/proxy_resolver_js_bindings.cc:104: int
DnsResolveHelper(HostResolver::RequestInfo& info,
Should 'info' be a const reference?

http://codereview.chromium.org/2833021/diff/13001/14003
File net/proxy/proxy_resolver_js_bindings.h (right):

http://codereview.chromium.org/2833021/diff/13001/14003#newcode64
net/proxy/proxy_resolver_js_bindings.h:64: ProxyResolverRequestContext*
current_request_context() {
Should the getter be 'const'?

http://codereview.chromium.org/2833021/diff/13001/14004
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/2833021/diff/13001/14004#newcode236
net/proxy/proxy_resolver_js_bindings_unittest.cc:236: 
Nit: extraneous blank line.

http://codereview.chromium.org/2833021/diff/13001/14004#newcode252
net/proxy/proxy_resolver_js_bindings_unittest.cc:252: // The "Ex" version shares
this same cache, however since the flags
Why don't you call host_resolver->ResetCount() before this
experiment?  It's intentional to show a new entry is created
and reused for the subsequent three DnsResolveEx calls, right?

http://codereview.chromium.org/2833021/diff/13001/14005
File net/proxy/proxy_resolver_request_context.h (right):

http://codereview.chromium.org/2833021/diff/13001/14005#newcode13
net/proxy/proxy_resolver_request_context.h:13: // This datastructure holds state
relating to an invocation of
Nit: datastructure => data structure
     relating to => related to

Consider defining this structure as a class with all
public members, so that when you forward-declare this type
you don't need to remember it is a struct.

http://codereview.chromium.org/2833021/diff/13001/14005#newcode15
net/proxy/proxy_resolver_request_context.h:15: // data (such as the motivating
URL, and the netlog for the request).
Remove "the motivating URL" in this comment and the
query_url member, because query_url is not being used.

[eroman, 2010-06-22 03:16:04]

Thanks for the review.

http://codereview.chromium.org/2833021/diff/13001/14002
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/2833021/diff/13001/14002#newcode104
net/proxy/proxy_resolver_js_bindings.cc:104: int
DnsResolveHelper(HostResolver::RequestInfo& info,
On 2010/06/22 02:24:17, wtc wrote:
> Should 'info' be a const reference?

Yes it should. Fixed.

http://codereview.chromium.org/2833021/diff/13001/14003
File net/proxy/proxy_resolver_js_bindings.h (right):

http://codereview.chromium.org/2833021/diff/13001/14003#newcode64
net/proxy/proxy_resolver_js_bindings.h:64: ProxyResolverRequestContext*
current_request_context() {
On 2010/06/22 02:24:17, wtc wrote:
> Should the getter be 'const'?

In the past Will has asked me not to make getters that return non-const pointers
as "const". I can make it const if you would prefer though.

http://codereview.chromium.org/2833021/diff/13001/14004
File net/proxy/proxy_resolver_js_bindings_unittest.cc (right):

http://codereview.chromium.org/2833021/diff/13001/14004#newcode236
net/proxy/proxy_resolver_js_bindings_unittest.cc:236: 
On 2010/06/22 02:24:17, wtc wrote:
> Nit: extraneous blank line.

Done.

http://codereview.chromium.org/2833021/diff/13001/14004#newcode252
net/proxy/proxy_resolver_js_bindings_unittest.cc:252: // The "Ex" version shares
this same cache, however since the flags
On 2010/06/22 02:24:17, wtc wrote:
> Why don't you call host_resolver->ResetCount() before this
> experiment?  It's intentional to show a new entry is created
> and reused for the subsequent three DnsResolveEx calls, right?

Done.

http://codereview.chromium.org/2833021/diff/13001/14005
File net/proxy/proxy_resolver_request_context.h (right):

http://codereview.chromium.org/2833021/diff/13001/14005#newcode15
net/proxy/proxy_resolver_request_context.h:15: // data (such as the motivating
URL, and the netlog for the request).
On 2010/06/22 02:24:17, wtc wrote:
> Remove "the motivating URL" in this comment and the
> query_url member, because query_url is not being used.

Done.

[wtc, 2010-06-23 00:59:02]

LGTM.  Note the issue below.

http://codereview.chromium.org/2833021/diff/21001/7006
File net/proxy/proxy_resolver_request_context.h (right):

http://codereview.chromium.org/2833021/diff/21001/7006#newcode27
net/proxy/proxy_resolver_request_context.h:27: const GURL* query_url;
You forgot to remove the query_url, which I believe is unused.