Avoid doing concurrent DNS resolves of the same hostname

* Avoid doing concurrent DNS resolves of the same hostname in HostResolver.

* Add a 1 minute cache for host resolves.

* Refactor HostResolver to handle multiple requests.

* Make HostResolver a dependency of URLRequestContext. operate the HostResolver
in async mode for proxy resolver (bridging to IO thread).

TEST=unittests
BUG=13163
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18236

[eroman, 2009-06-04 14:07:19]

This is mostly ready for review, although still need to address Darin's comments
regarding how to use a factory.

Also need to upload a version synced to head (i tried doing that now but got a
bunch of conflicts, so I won't finish that "tonight").

The important bits for review (in order) are:

  host_resolver.h
  host_resolver.cc
  host_resolver_unittest.cc
  address_list.h
  address_list.cc
  proxy_resolver_v8.h
  proxy_resolver_v8.cc

Not sure how to break up reviewer responsibilities... i guess whoever has "free
time" of darin/wtc.

[eroman, 2009-06-04 14:09:36]

http://codereview.chromium.org/118100/diff/1156/302
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/1156/302#newcode22
Line 22: struct addrinfo* CreateCopyOfAddrinfo(struct addrinfo* info) {
... hopefully I am just tired and there is a better way of doing this, since
these next three methods are pretty absurd.

[eroman, 2009-06-05 21:09:49]

Updated: no longer makes another copy of addrinfo if the ports match (will
refcount the data instead).

[wtc, 2009-06-09 02:23:40]

LGTM.

http://codereview.chromium.org/118100/diff/570/602
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/570/602#newcode23
Line 23: struct addrinfo* copy = new addrinfo;
Nit: does "new struct addrinfo" work?

http://codereview.chromium.org/118100/diff/570/602#newcode30
Line 30: copy->ai_canonname = _strdup(info->ai_canonname);
_strdup is the name of this function on Windows.

On Mac and Linux, this function is named strdup.

Windows also provides the name strdup, so you can just use
strdup on all platforms.

http://codereview.chromium.org/118100/diff/570/602#newcode51
Line 51: delete reinterpret_cast<char*>(info->ai_addr);
IMPORTANT: This should be delete [].

http://codereview.chromium.org/118100/diff/570/603
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/570/603#newcode26
Line 26: // Sets the port of all address in the list to |port| (that is the
Nit: address => addresses

http://codereview.chromium.org/118100/diff/570/603#newcode30
Line 30: // Retrieves the port number of the first sockaddr in the list. (If
SetPort()
I think all the addresses have the same port number whether
SetPort() was previously used on this list or not, right?

http://codereview.chromium.org/118100/diff/570/603#newcode32
Line 32: // same port number.
Nit: the closing parenthesis is missing.

http://codereview.chromium.org/118100/diff/570/603#newcode39
Line 39: struct Data : public base::RefCountedThreadSafe<Data> {
Nit: once this struct has a private data member, it isn't
following the style guide any more.  Perhaps we should change
it to a class and provide a head() getter?

http://codereview.chromium.org/118100/diff/570/608
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/118100/diff/570/608#newcode73
Line 73: // Returns the HostResovler that will be used for DNS lookups.
Nit: DNS is just one of the many hostname resolution methods.

http://codereview.chromium.org/118100/diff/570/596
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/570/596#newcode147
Line 147: int err = getaddrinfo(host.c_str(), NULL, &hints, out);
Are you sure we can pass NULL as the second argument to
getaddrinfo?

[willchan no longer on Chromium, 2009-06-09 05:42:05]

Drive by.  Was curious about the work you were doing here so decided to take a
look.  Looks like a big improvement :)  Had some minor nitpicks and possible
suggestions on better testing coverage.

http://codereview.chromium.org/118100/diff/570/602
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/570/602#newcode65
Line 65: DCHECK(info->ai_addrlen == sizeof(sockaddr_in));
DCHECK_EQ

http://codereview.chromium.org/118100/diff/570/602#newcode70
Line 70: DCHECK(info->ai_addrlen == sizeof(sockaddr_in6));
DCHECK_EQ

http://codereview.chromium.org/118100/diff/570/603
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/570/603#newcode48
Line 48: bool is_system_created_;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

This should not have a trailing underscore.

http://codereview.chromium.org/118100/diff/570/608
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/118100/diff/570/608#newcode73
Line 73: // Returns the HostResovler that will be used for DNS lookups.
s/HostResovler/HostResolver/

http://codereview.chromium.org/118100/diff/570/596
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/570/596#newcode180
Line 180: Request(CompletionCallback* callback, AddressList* addresses, int
port)
Nitpicky, but output params should be last:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/118100/diff/570/596#newcode190
Line 190: bool was_cancelled() {
looks like this method can be const

http://codereview.chromium.org/118100/diff/570/596#newcode194
Line 194: void set_job(ResolveJob* job) {
I wonder if it's worth adding a DCHECK(job_ != NULL) here.

http://codereview.chromium.org/118100/diff/570/596#newcode223
Line 223: class HostResolver::ResolveJob :
I think you have a lint error here.  Have you run lint?

http://codereview.chromium.org/118100/diff/570/596#newcode239
Line 239: // Free the requests attached to this job.
How about replacing this with STLDeleteElements(&requests_);?

http://codereview.chromium.org/118100/diff/570/596#newcode301
Line 301: typedef std::vector<Request*> RequestsList;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

typedefs go first in the public/protected/private sections.

http://codereview.chromium.org/118100/diff/570/596#newcode328
Line 328: // Cancel the outsanding and pending jobs. Those jobs may contain
several
s/outsanding/outstanding/

http://codereview.chromium.org/118100/diff/570/596#newcode391
Line 391: const std::string& hostname) {
This looks like it can be const.

http://codereview.chromium.org/118100/diff/570/596#newcode402
Line 402: HostResolver::ResolveJobs::iterator HostResolver::FindJob(ResolveJob*
job) {
This seems like it could be const too.

http://codereview.chromium.org/118100/diff/570/596#newcode417
Line 417: // Remove the job from queue (keep a reference to postpone the final
deref).
It looks to me like you do this to prevent new requests that might get created
during the callbacks' execution from getting added to this job.  It might be
nice to add a comment explaining this.

http://codereview.chromium.org/118100/diff/570/596#newcode423
Line 423: bool has_addrlist = error == OK;
I wonder if it's worth adding a comment beyond this point saying that it's not
valid to access any member variables beyond this point since it's possible that
the HostResolver gets deleted by one of the callbacks below.  If you do that,
then I might suggest pulling the rest of the code in this member function into a
non-member function, so it's clear that there are no further access to member
variables.

http://codereview.chromium.org/118100/diff/570/596#newcode432
Line 432: it != job->requests_.end(); ++it) {
It looks like this private access is the reason you need HostResolver to be a
friend of ResolveJob.  What if you changed the signature of
HostResolver::OnJobCompleted() to take a const ResolveJob::RequestsList&, and a
struct addrinfo*?  This is just my personal preference because I have a bias
against friend declarations in general.  If you prefer the way it is already,
that's fine too.

http://codereview.chromium.org/118100/diff/570/596#newcode435
Line 435: DCHECK(req->job_ == job);
DCHECK_EQ

http://codereview.chromium.org/118100/diff/570/596#newcode475
Line 475: MessageLoop::current()->PostTask(FROM_HERE,
you should move FROM_HERE to the next line to keep all the parameters aligned

http://codereview.chromium.org/118100/diff/570/594
File net/base/host_resolver.h (right):

http://codereview.chromium.org/118100/diff/570/594#newcode89
Line 89: friend class ResolveJob;
Do you need this friend class declaration?  Private inner classes already have
private access to the containing class.

http://codereview.chromium.org/118100/diff/570/595
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/570/595#newcode51
Line 51: std::vector<std::string> GetCaptureList() {
const?

http://codereview.chromium.org/118100/diff/570/595#newcode55
Line 55: copy = capture_list_;
Do you need all these lines?  I think can get rid of the copy local variable,
and just return capture_list_ here.

http://codereview.chromium.org/118100/diff/570/595#newcode67
Line 67: // resolve results in CompletionDelegate.
What's CompletionDelegate?

http://codereview.chromium.org/118100/diff/570/595#newcode70
Line 70: 
I'm not sure if this newline is intentional, but I don't usually see one after
public:

http://codereview.chromium.org/118100/diff/570/595#newcode75
Line 75: virtual ~Delegate() {}
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
The destructor should go first.

Also, some of the new classes are missing DISALLOW_COPY_AND_ASSIGN.  Not that
big a deal since they're in .cc files, but I usually add them anyway.

http://codereview.chromium.org/118100/diff/570/595#newcode294
Line 294: static int count_b = 0;
Can you just make these member variables?  It seems you use the same
DeDupeRequestsVerifier instance across all requests in your test.

http://codereview.chromium.org/118100/diff/570/595#newcode364
Line 364: EXPECT_EQ(resolve->hostname, "a");
"a" should be first in order to get the "expected" and "actual" messages correct
in gTest.  I suspect you're doing this because of the operator== in std:string
causing "a" to implicitly get converted into a std::string.  I suggest instead
doing EXPECT_STREQ("a", resolve->hostname.c_str());

http://codereview.chromium.org/118100/diff/570/595#newcode411
Line 411: 
I wonder if it's worth adding tests that do CancelRequest() within the callback
passed to HostResolver.

And maybe one that deletes the HostResolver itself.

[eroman, 2009-06-09 11:15:16]

Thanks for the reviews.

It turns out I had made some additional changes in my client which have not
become of the latest patchset. Sorry about that! but it is sticky to remove it
now, so hopefully it can just be absorbed into this review. Adds some new files
(host_cache.h, host_cache.cc, host_cache_unittest.cc, address_list_unittest.cc),
and turns on the 1 minute cache.

http://codereview.chromium.org/118100/diff/570/602
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/570/602#newcode23
Line 23: struct addrinfo* copy = new addrinfo;
On 2009/06/09 02:23:40, wtc wrote:
> Nit: does "new struct addrinfo" work?

Done.

http://codereview.chromium.org/118100/diff/570/602#newcode30
Line 30: copy->ai_canonname = _strdup(info->ai_canonname);
On 2009/06/09 02:23:40, wtc wrote:
> _strdup is the name of this function on Windows.
> 
> On Mac and Linux, this function is named strdup.
> 
> Windows also provides the name strdup, so you can just use
> strdup on all platforms.

Wrapped it in an #ifdef.
(need to use the underscore version on windows, since it warns of deprecation).

http://codereview.chromium.org/118100/diff/570/602#newcode51
Line 51: delete reinterpret_cast<char*>(info->ai_addr);
On 2009/06/09 02:23:40, wtc wrote:
> IMPORTANT: This should be delete [].

Done.
Good spot.

Added "address_list_unittest.cc" to verify.
(interestingly seems to have been passing in purify without the [] too).

http://codereview.chromium.org/118100/diff/570/602#newcode65
Line 65: DCHECK(info->ai_addrlen == sizeof(sockaddr_in));
On 2009/06/09 05:42:06, willchan wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/118100/diff/570/602#newcode70
Line 70: DCHECK(info->ai_addrlen == sizeof(sockaddr_in6));
On 2009/06/09 05:42:06, willchan wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/118100/diff/570/603
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/570/603#newcode26
Line 26: // Sets the port of all address in the list to |port| (that is the
On 2009/06/09 02:23:40, wtc wrote:
> Nit: address => addresses

Done.

http://codereview.chromium.org/118100/diff/570/603#newcode30
Line 30: // Retrieves the port number of the first sockaddr in the list. (If
SetPort()
On 2009/06/09 02:23:40, wtc wrote:
> I think all the addresses have the same port number whether
> SetPort() was previously used on this list or not, right?

Yes (assuming we don't pass in some wonkily constructed addrinfo).

Should I change the comment?

http://codereview.chromium.org/118100/diff/570/603#newcode32
Line 32: // same port number.
On 2009/06/09 02:23:40, wtc wrote:
> Nit: the closing parenthesis is missing.

Done.

http://codereview.chromium.org/118100/diff/570/603#newcode39
Line 39: struct Data : public base::RefCountedThreadSafe<Data> {
On 2009/06/09 02:23:40, wtc wrote:
> Nit: once this struct has a private data member, it isn't
> following the style guide any more.  Perhaps we should change
> it to a class and provide a head() getter?

Done.
Made the |is_system_created| public to match semantics of struct.

http://codereview.chromium.org/118100/diff/570/603#newcode48
Line 48: bool is_system_created_;
On 2009/06/09 05:42:06, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...
> 
> This should not have a trailing underscore.

Done.
(per wtc's comment, no longer private).

http://codereview.chromium.org/118100/diff/570/608
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/118100/diff/570/608#newcode73
Line 73: // Returns the HostResovler that will be used for DNS lookups.
On 2009/06/09 02:23:40, wtc wrote:
> Nit: DNS is just one of the many hostname resolution methods.

Done.

(Also fixed some other comments where I made this same inaccurate
generalization.)

http://codereview.chromium.org/118100/diff/570/608#newcode73
Line 73: // Returns the HostResovler that will be used for DNS lookups.
On 2009/06/09 05:42:06, willchan wrote:
> s/HostResovler/HostResolver/

Done.

http://codereview.chromium.org/118100/diff/570/596
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/570/596#newcode147
Line 147: int err = getaddrinfo(host.c_str(), NULL, &hints, out);
On 2009/06/09 02:23:40, wtc wrote:
> Are you sure we can pass NULL as the second argument to
> getaddrinfo?

Yes, NULL is an accepted parameter (both linux/windows).

In fact that is how IE8 is calling getaddrinfo (see http://crbug.com/11520).

http://codereview.chromium.org/118100/diff/570/596#newcode180
Line 180: Request(CompletionCallback* callback, AddressList* addresses, int
port)
On 2009/06/09 05:42:06, willchan wrote:
> Nitpicky, but output params should be last:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

What do you mean by "output parameter"? this is a constructor

http://codereview.chromium.org/118100/diff/570/596#newcode190
Line 190: bool was_cancelled() {
On 2009/06/09 05:42:06, willchan wrote:
> looks like this method can be const

Done.

http://codereview.chromium.org/118100/diff/570/596#newcode194
Line 194: void set_job(ResolveJob* job) {
On 2009/06/09 05:42:06, willchan wrote:
> I wonder if it's worth adding a DCHECK(job_ != NULL) here.

Done.

http://codereview.chromium.org/118100/diff/570/596#newcode223
Line 223: class HostResolver::ResolveJob :
On 2009/06/09 05:42:06, willchan wrote:
> I think you have a lint error here.  Have you run lint?

No longer applicable.

http://codereview.chromium.org/118100/diff/570/596#newcode239
Line 239: // Free the requests attached to this job.
On 2009/06/09 05:42:06, willchan wrote:
> How about replacing this with STLDeleteElements(&requests_);?

Done.
Neat, didn't know about that.

http://codereview.chromium.org/118100/diff/570/596#newcode301
Line 301: typedef std::vector<Request*> RequestsList;
On 2009/06/09 05:42:06, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> typedefs go first in the public/protected/private sections.

Done.

http://codereview.chromium.org/118100/diff/570/596#newcode328
Line 328: // Cancel the outsanding and pending jobs. Those jobs may contain
several
On 2009/06/09 05:42:06, willchan wrote:
> s/outsanding/outstanding/

Done.

http://codereview.chromium.org/118100/diff/570/596#newcode391
Line 391: const std::string& hostname) {
On 2009/06/09 05:42:06, willchan wrote:
> This looks like it can be const.

No longer applicable (this functionality is part of HostCache now).

http://codereview.chromium.org/118100/diff/570/596#newcode402
Line 402: HostResolver::ResolveJobs::iterator HostResolver::FindJob(ResolveJob*
job) {
On 2009/06/09 05:42:06, willchan wrote:
> This seems like it could be const too.

No longer applicable (this functionality is no longer needed due to HostCache).

http://codereview.chromium.org/118100/diff/570/596#newcode417
Line 417: // Remove the job from queue (keep a reference to postpone the final
deref).
On 2009/06/09 05:42:06, willchan wrote:
> It looks to me like you do this to prevent new requests that might get created
> during the callbacks' execution from getting added to this job.  It might be
> nice to add a comment explaining this.

This is no longer applicable (part of HostCache).

http://codereview.chromium.org/118100/diff/570/596#newcode423
Line 423: bool has_addrlist = error == OK;
On 2009/06/09 05:42:06, willchan wrote:
> I wonder if it's worth adding a comment beyond this point saying that it's not
> valid to access any member variables beyond this point since it's possible
that
> the HostResolver gets deleted by one of the callbacks below.  If you do that,
> then I might suggest pulling the rest of the code in this member function into
a
> non-member function, so it's clear that there are no further access to member
> variables.

Changed the function around a bit, it is now in HostResolver::OnLookupComplete.

Added comments explaining about HostResolver's possible death during callback.

http://codereview.chromium.org/118100/diff/570/596#newcode432
Line 432: it != job->requests_.end(); ++it) {
On 2009/06/09 05:42:06, willchan wrote:
> It looks like this private access is the reason you need HostResolver to be a
> friend of ResolveJob.  What if you changed the signature of
> HostResolver::OnJobCompleted() to take a const ResolveJob::RequestsList&, and
a
> struct addrinfo*?  This is just my personal preference because I have a bias
> against friend declarations in general.  If you prefer the way it is already,
> that's fine too.

This is moved into HostResolverJob::OnLookupComplete().
I have removed the friends- they were mostly used to access private stuff from
DCHECKs, instead I have added accessors for all of those.

http://codereview.chromium.org/118100/diff/570/596#newcode435
Line 435: DCHECK(req->job_ == job);
On 2009/06/09 05:42:06, willchan wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/118100/diff/570/596#newcode475
Line 475: MessageLoop::current()->PostTask(FROM_HERE,
On 2009/06/09 05:42:06, willchan wrote:
> you should move FROM_HERE to the next line to keep all the parameters aligned

Done.

http://codereview.chromium.org/118100/diff/570/595
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/570/595#newcode51
Line 51: std::vector<std::string> GetCaptureList() {
On 2009/06/09 05:42:06, willchan wrote:
> const?

Can't, AutoLock needs a non-const lock.

http://codereview.chromium.org/118100/diff/570/595#newcode55
Line 55: copy = capture_list_;
On 2009/06/09 05:42:06, willchan wrote:
> Do you need all these lines?  I think can get rid of the copy local variable,
> and just return capture_list_ here.

I found it confusing to remove the brackets, since it isn't obvious whether the
copy constructor gets called before/after the autolock goes out of scope.

http://codereview.chromium.org/118100/diff/570/595#newcode67
Line 67: // resolve results in CompletionDelegate.
On 2009/06/09 05:42:06, willchan wrote:
> What's CompletionDelegate?

Should have read Delegate. Fixed.

http://codereview.chromium.org/118100/diff/570/595#newcode70
Line 70: 
On 2009/06/09 05:42:06, willchan wrote:
> I'm not sure if this newline is intentional, but I don't usually see one after
> public:

Done- removed newline.

http://codereview.chromium.org/118100/diff/570/595#newcode75
Line 75: virtual ~Delegate() {}
On 2009/06/09 05:42:06, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> The destructor should go first.
> 
> Also, some of the new classes are missing DISALLOW_COPY_AND_ASSIGN.  Not that
> big a deal since they're in .cc files, but I usually add them anyway.

Done.

http://codereview.chromium.org/118100/diff/570/595#newcode294
Line 294: static int count_b = 0;
On 2009/06/09 05:42:06, willchan wrote:
> Can you just make these member variables?  It seems you use the same
> DeDupeRequestsVerifier instance across all requests in your test.

Done.

http://codereview.chromium.org/118100/diff/570/595#newcode364
Line 364: EXPECT_EQ(resolve->hostname, "a");
On 2009/06/09 05:42:06, willchan wrote:
> "a" should be first in order to get the "expected" and "actual" messages
correct
> in gTest.  I suspect you're doing this because of the operator== in std:string
> causing "a" to implicitly get converted into a std::string.  I suggest instead
> doing EXPECT_STREQ("a", resolve->hostname.c_str());

Done.

http://codereview.chromium.org/118100/diff/570/595#newcode411
Line 411: 
On 2009/06/09 05:42:06, willchan wrote:
> I wonder if it's worth adding tests that do CancelRequest() within the
callback
> passed to HostResolver.

Done -- added HostResolverTest.CancelWithinCallback

> 
> And maybe one that deletes the HostResolver itself.

Done -- added HostResolverTest.DeleteWithinCallback

Also added the following additional tests:

   HostResolverTest.StartWithinCallback
   HostCacheTest.Basic
   HostCacheTest.NegativeEntry
   HostCacheTest.Compact
   HostCacheTest.NoCache
   AddressListTest.GetPort
   AddressListTest.Assignment
   AddressListTest.Copy

There are one or two other edge cases I want to test that I have left as TODO.

willchan: I disable host resolver caching for
tcp_client_socket_pool_unittest.cc, since those tests fail when HostResolver
completes synchronously. Can you take a look?

[eroman, 2009-06-09 11:17:32]

> It turns out I had made some additional changes in my client
> which have not become of the latest patchset.

Typo: "which have not" --> "which have now"

[darin (slow to review), 2009-06-09 18:43:25]

I have only looked address_list.h.  Will review more today or tomorrow.  Sorry
for the delay!

http://codereview.chromium.org/118100/diff/1509/1542
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/1509/1542#newcode24
Line 24: void Copy(const struct addrinfo* head);
I created AddressList::Data as a reference counted container for addrinfo.  That
should mean that we can never need to copy addrinfo since addrinfo is immutable.

I haven't looked carefully to see why you need Copy.

[eroman, 2009-06-09 20:05:02]

http://codereview.chromium.org/118100/diff/1509/1542
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/1509/1542#newcode24
Line 24: void Copy(const struct addrinfo* head);
On 2009/06/09 18:43:25, darin wrote:
> I created AddressList::Data as a reference counted container for addrinfo. 
That
> should mean that we can never need to copy addrinfo since addrinfo is
immutable.
> 
> I haven't looked carefully to see why you need Copy.

The host resolver cache saves an AddressList per hostname. When a resolve for
the same hostname is requested but with a different port number, then this is
served from the cache by making a copy of the cached AddressList and changing
the copy's port number. Otherwise it just addrefs the existing Data.

[darin (slow to review), 2009-06-09 20:41:08]

Ah, the port number! :(

[willchan no longer on Chromium, 2009-06-10 02:11:28]

I think I still prefer maintaining a separate hash_map in the HostResolver that
tracks the ResolveJobs instead of having the HostCache do it.  I think it's
worth hearing what darin and wtc have to say though.  In absence of their
comments, I'm fine with your current implementation of HostCache (given that you
plan to bound the number of concurrent requests), and revisiting later as you
add histograms and play around with different cache policies.

That said, do you think it's an issue to check in the code without the
concurrent request limits?  Is it a major issue if we hit that DLOG(WARNING)
you've left there?  It seems like an edge case that's very minor, and this isn't
going to go into a major release branch, so it's probably fine.

Other than that, I've just got nitpicks.  Is TCPClientSocketPoolTest giving you
problems still?  Should I download your patch and take a look?  If I submit my
refactor which you lgtm'd, will that cause you problems?  Or should I hold off?

http://codereview.chromium.org/118100/diff/570/595
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/570/595#newcode51
Line 51: std::vector<std::string> GetCaptureList() {
On 2009/06/09 11:15:17, eroman wrote:
> On 2009/06/09 05:42:06, willchan wrote:
> > const?
> 
> Can't, AutoLock needs a non-const lock.

How about making the lock mutable?  That's commonly done.

http://codereview.chromium.org/118100/diff/1509/1541
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/1509/1541#newcode31
Line 31: copy->ai_canonname = _strdup(info->ai_canonname);
Didn't wtc say that windows also provides strdup(), so you don't need to #ifdef
and use _strdup?

http://codereview.chromium.org/118100/diff/1509/1542
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/1509/1542#newcode56
Line 56: private:
I don't actually think the private section and the zero-arg constructor are
necessary.  A default constructor should not be automatically generated if
another constructor is provided.  In this case you already have a two-arg
constructor.  This is at least true for gcc, I dunno about msvc.

http://codereview.chromium.org/118100/diff/1509/1554
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1554#newcode29
Line 29: std::string host = "192.168.1.1";
Why are you using a string instead of a string literal?  const char kHost[] =
"192.168.1.1"

http://codereview.chromium.org/118100/diff/1509/1553
File net/base/host_cache.h (right):

http://codereview.chromium.org/118100/diff/1509/1553#newcode48
Line 48: HostCache* parent_;
Feels weird to me to call it parent.  I'd just call it cache_ or something.

http://codereview.chromium.org/118100/diff/1509/1553#newcode53
Line 53: // responsible for pouplating it.
s/pouplating/populating/

http://codereview.chromium.org/118100/diff/1509/1552
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1552#newcode16
Line 16: TEST(HostCacheTest, Basic) {
Perhaps this should be named Expiration instead of Basic.

http://codereview.chromium.org/118100/diff/1509/1552#newcode139
Line 139: EXPECT_TRUE(cache.entries_.find("valid0") == cache.entries_.end());
EXPECT_TRUE(!ContainsKey(cache.entries_, "valid0"));

http://codereview.chromium.org/118100/diff/1509/1535
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/1509/1535#newcode201
Line 201: void InvokeCallback(HostCacheEntry* cache_entry) {
Part of me wonders if this should be called OnRequestComplete() or something. 
Meh, it's fine as is.

http://codereview.chromium.org/118100/diff/1509/1535#newcode216
Line 216: 
Unnecessary newline.

http://codereview.chromium.org/118100/diff/1509/1535#newcode228
Line 228: };
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/118100/diff/1509/1535#newcode278
Line 278: // Running on the origin thread.
DCHECK_EQ(origin_loop_, MessageLoop::current())?

http://codereview.chromium.org/118100/diff/1509/1535#newcode368
Line 368: // though we may complete before it.
You _could_ use a WaitableEvent in the ResolveJob, but it's probably overkill. 
I can't imagine this case comes up much.

http://codereview.chromium.org/118100/diff/1509/1535#newcode413
Line 413: void HostResolver::StartJob(HostResolverJob* job) {
HostResolverJob::DoLookup() and HostResolverJob::OnLookupComplete() and error_
all seem like they should be private.  What if you made a
HostResolverJob::Start() function that did this?  It looks like you want this
function mainly as a chokepoint for your TODO here.  If so, I'd just take the
code on 390-396 and move it here and call this function CreateNewJob() instead.

http://codereview.chromium.org/118100/diff/1509/1533
File net/base/host_resolver.h (right):

http://codereview.chromium.org/118100/diff/1509/1533#newcode92
Line 92: // non-NULL, then *req will be filled with a handle to the async
request.
|*req|

http://codereview.chromium.org/118100/diff/1509/1534
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1534#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Nice test coverage in this file.

http://codereview.chromium.org/118100/diff/1509/1534#newcode98
Line 98: // The request details.
Sort of annoying, but since this is a class, these should be private as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access....

http://codereview.chromium.org/118100/diff/1509/1534#newcode428
Line 428: // Port 80 is the first request that the callback will be inoked for.
s/inoked/invoked/
Did you document that request callbacks are run in the order that they were
added in?

http://codereview.chromium.org/118100/diff/1509/1534#newcode574
Line 574: if (num_requests_++ == 4) {
no need for the braces
what's this code for btw?  it seems like the above Quit() will always get
called, so we don't need this second if statement, do we?

[eroman, 2009-06-10 17:53:13]

> I think I still prefer maintaining a separate hash_map in the HostResolver
that
> tracks the ResolveJobs instead of having the HostCache do it.  I think it's
> worth hearing what darin and wtc have to say though.  In absence of their
> comments, I'm fine with your current implementation of HostCache (given that
you
> plan to bound the number of concurrent requests), and revisiting later as you
> add histograms and play around with different cache policies.

Ok, you have convinced me!

I had originally embedded the HostResolverJob into the cache entry as an
optimization -- that way end-to-end resolves would just require a single hashmap
access (cache entries were pinned while the HostResolverJob was outstanding so
on completion it didn't need to re-lookup the entry).

If I lift out that optimization, end-to-end resolving will require 4x as many
hashmap operations -- (lookup cache entry, lookup job entry, update cache entry,
update job entry).

I agree this is a worthwhile tradeoff for code simplicity. It keeps the caching
layer completely separate from the job layer, which makes it easier to
understand and change the code.

I will make this change shortly.

> That said, do you think it's an issue to check in the code without the
> concurrent request limits?  Is it a major issue if we hit that DLOG(WARNING)
> you've left there?  It seems like an edge case that's very minor, and this
isn't
> going to go into a major release branch, so it's probably fine.

The current code doesn't limit concurrent resolves either, so this is no
different.
It is something I want to address though, but not part of this change, so the
TODO should be safe.

[willchan no longer on Chromium, 2009-06-10 21:04:21]

On 2009/06/10 17:53:13, eroman wrote:
> > I think I still prefer maintaining a separate hash_map in the HostResolver
> that
> > tracks the ResolveJobs instead of having the HostCache do it.  I think it's
> > worth hearing what darin and wtc have to say though.  In absence of their
> > comments, I'm fine with your current implementation of HostCache (given that
> you
> > plan to bound the number of concurrent requests), and revisiting later as
you
> > add histograms and play around with different cache policies.
> 
> Ok, you have convinced me!
> 
> I had originally embedded the HostResolverJob into the cache entry as an
> optimization -- that way end-to-end resolves would just require a single
hashmap
> access (cache entries were pinned while the HostResolverJob was outstanding so
> on completion it didn't need to re-lookup the entry).
> 
> If I lift out that optimization, end-to-end resolving will require 4x as many
> hashmap operations -- (lookup cache entry, lookup job entry, update cache
entry,
> update job entry).
> 
> I agree this is a worthwhile tradeoff for code simplicity. It keeps the
caching
> layer completely separate from the job layer, which makes it easier to
> understand and change the code.
> 
> I will make this change shortly.

Sorry, I didn't mean to jerk you around.  I was happy to wait until what others
thought, but yeah, I think that the code simplicity is worth it.  I'm not too
particularly concerned with the extra hash lookup costs.  If it shows up on
profiles, then we can revisit.

> 
> > That said, do you think it's an issue to check in the code without the
> > concurrent request limits?  Is it a major issue if we hit that DLOG(WARNING)
> > you've left there?  It seems like an edge case that's very minor, and this
> isn't
> > going to go into a major release branch, so it's probably fine.
> 
> The current code doesn't limit concurrent resolves either, so this is no
> different.
> It is something I want to address though, but not part of this change, so the
> TODO should be safe.

[eroman, 2009-06-11 00:07:27]

I believe I have addressed all your comments.
(deltas are patchset 15 --> patchset 19).

I will sync the latest client socket pool changes as a separate patchset.

http://codereview.chromium.org/118100/diff/570/595
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/570/595#newcode51
Line 51: std::vector<std::string> GetCaptureList() {
On 2009/06/10 02:11:29, willchan wrote:
> On 2009/06/09 11:15:17, eroman wrote:
> > On 2009/06/09 05:42:06, willchan wrote:
> > > const?
> > 
> > Can't, AutoLock needs a non-const lock.
> 
> How about making the lock mutable?  That's commonly done.

Done.

http://codereview.chromium.org/118100/diff/1509/1541
File net/base/address_list.cc (right):

http://codereview.chromium.org/118100/diff/1509/1541#newcode31
Line 31: copy->ai_canonname = _strdup(info->ai_canonname);
On 2009/06/10 02:11:29, willchan wrote:
> Didn't wtc say that windows also provides strdup(), so you don't need to
#ifdef
> and use _strdup?

I need the ifdef because strdup() is deprecated on msvc. It gives warning (and
we error on warnings).

http://codereview.chromium.org/118100/diff/1509/1542
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/1509/1542#newcode56
Line 56: private:
On 2009/06/10 02:11:29, willchan wrote:
> I don't actually think the private section and the zero-arg constructor are
> necessary.  A default constructor should not be automatically generated if
> another constructor is provided.  In this case you already have a two-arg
> constructor.  This is at least true for gcc, I dunno about msvc.

Removed.

http://codereview.chromium.org/118100/diff/1509/1554
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1554#newcode29
Line 29: std::string host = "192.168.1.1";
On 2009/06/10 02:11:29, willchan wrote:
> Why are you using a string instead of a string literal?  const char kHost[] =
> "192.168.1.1"

Done.

http://codereview.chromium.org/118100/diff/1509/1553
File net/base/host_cache.h (right):

http://codereview.chromium.org/118100/diff/1509/1553#newcode48
Line 48: HostCache* parent_;
On 2009/06/10 02:11:29, willchan wrote:
> Feels weird to me to call it parent.  I'd just call it cache_ or something.

Removed (there is no longer need for |parent| now that unflattened the layer).

http://codereview.chromium.org/118100/diff/1509/1553#newcode53
Line 53: // responsible for pouplating it.
On 2009/06/10 02:11:29, willchan wrote:
> s/pouplating/populating/

Done.

http://codereview.chromium.org/118100/diff/1509/1553#newcode53
Line 53: // responsible for pouplating it.
On 2009/06/10 02:11:29, willchan wrote:
> s/pouplating/populating/

Done.
(not yet uploaded fix tho).

http://codereview.chromium.org/118100/diff/1509/1552
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1552#newcode16
Line 16: TEST(HostCacheTest, Basic) {
On 2009/06/10 02:11:29, willchan wrote:
> Perhaps this should be named Expiration instead of Basic.

I have left it as Basic for now -- it tests Lookup(), Set(), size(), and
expiration. I can rename if you think Expiration is still more approriate.

http://codereview.chromium.org/118100/diff/1509/1552#newcode139
Line 139: EXPECT_TRUE(cache.entries_.find("valid0") == cache.entries_.end());
On 2009/06/10 02:11:29, willchan wrote:
> EXPECT_TRUE(!ContainsKey(cache.entries_, "valid0"));

No longer applicable (interface of cache is just Lookup() and Set() now).

http://codereview.chromium.org/118100/diff/1509/1535
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/1509/1535#newcode201
Line 201: void InvokeCallback(HostCacheEntry* cache_entry) {
On 2009/06/10 02:11:29, willchan wrote:
> Part of me wonders if this should be called OnRequestComplete() or something. 
> Meh, it's fine as is.

Done.

This is now:

OnComplete(int error, const AddressList& addrlist);

http://codereview.chromium.org/118100/diff/1509/1535#newcode216
Line 216: 
On 2009/06/10 02:11:29, willchan wrote:
> Unnecessary newline.

Done.

http://codereview.chromium.org/118100/diff/1509/1535#newcode228
Line 228: };
On 2009/06/10 02:11:29, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/118100/diff/1509/1535#newcode278
Line 278: // Running on the origin thread.
On 2009/06/10 02:11:29, willchan wrote:
> DCHECK_EQ(origin_loop_, MessageLoop::current())?
Done.

http://codereview.chromium.org/118100/diff/1509/1535#newcode368
Line 368: // though we may complete before it.
On 2009/06/10 02:11:29, willchan wrote:
> You _could_ use a WaitableEvent in the ResolveJob, but it's probably overkill.

> I can't imagine this case comes up much.

I no longer call this out in a comment. It is less weird now since the cache is
independent of the Job. I agree it is not worth optimizing this case.

http://codereview.chromium.org/118100/diff/1509/1535#newcode413
Line 413: void HostResolver::StartJob(HostResolverJob* job) {
On 2009/06/10 02:11:29, willchan wrote:
> HostResolverJob::DoLookup() and HostResolverJob::OnLookupComplete() and error_
> all seem like they should be private.  What if you made a
> HostResolverJob::Start() function that did this?  It looks like you want this
> function mainly as a chokepoint for your TODO here.  If so, I'd just take the
> code on 390-396 and move it here and call this function CreateNewJob()
instead.

Done.

Good call, I added a HostResolver::Job::Start() method, and made the other stuff
private.

http://codereview.chromium.org/118100/diff/1509/1533
File net/base/host_resolver.h (right):

http://codereview.chromium.org/118100/diff/1509/1533#newcode92
Line 92: // non-NULL, then *req will be filled with a handle to the async
request.
On 2009/06/10 02:11:29, willchan wrote:
> |*req|

Done.

http://codereview.chromium.org/118100/diff/1509/1534
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/118100/diff/1509/1534#newcode98
Line 98: // The request details.
On 2009/06/10 02:11:29, willchan wrote:
> Sort of annoying, but since this is a class, these should be private as per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access....

Done.

Was trying to cut corners in a test file :)

http://codereview.chromium.org/118100/diff/1509/1534#newcode428
Line 428: // Port 80 is the first request that the callback will be inoked for.
On 2009/06/10 02:11:29, willchan wrote:
> s/inoked/invoked/

Done.

> Did you document that request callbacks are run in the order that they were
> added in?

No. I am not sure that I want to call that out in documentation (but I do rely
on it for the tests to run through the proper edge cases.)

http://codereview.chromium.org/118100/diff/1509/1534#newcode574
Line 574: if (num_requests_++ == 4) {
On 2009/06/10 02:11:29, willchan wrote:
> no need for the braces
> what's this code for btw?  it seems like the above Quit() will always get
> called, so we don't need this second if statement, do we?

Ah, my mistake -- left over from older version.

This test is a bit different now- I re-defined Resolves() within callback to no
longer be sync, so the num_requests is now a necessary signal.

[willchan no longer on Chromium, 2009-06-11 08:25:15]

Sorry for the slowness.  I was out most of today in SF.  I didn't get to seeing
the new changes in the tests, although the host cache tests might change if you
agree with my suggestions.

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode23
Line 23: bool HostCache::Entry::can_use(const base::TimeTicks now) const {
Do you think it'd be easier for clients if we got rid of this method?  What if
Lookup() called this, and if it can't be used, it returns NULL?

http://codereview.chromium.org/118100/diff/5074/6065#newcode25
Line 25: // TODO(eroman): consider enabling use of negative entries.
What do you think about moving this error == OK check into HostCache::Set()
instead?  If so, then this function might even go away entirely, and you can add
the simple if statement in Compact() instead.  Note that this also saves space
in the cache.  Don't create the cache entry unless it can be used.

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
Do you think it'd be simpler if you didn't have to pin down this entry?  How
about checking entries_size.() == max_entries_ first, and doing the compaction
if necessary, before adding the new entry?

http://codereview.chromium.org/118100/diff/5074/6063
File net/base/host_cache.h (right):

http://codereview.chromium.org/118100/diff/5074/6063#newcode24
Line 24: class Entry : public base::RefCounted<Entry> {
How about making this a struct instead of a class?  It's used almost entirely
for its data, which you've made public and don't use trailing underscores in the
naming anyway.  I've made a suggestion elsewhere on eliminating can_use().

http://codereview.chromium.org/118100/diff/5074/6063#newcode48
Line 48: Entry* Lookup(const std::string& hostname);
Can this be const?  Return a const Entry* instead?

http://codereview.chromium.org/118100/diff/5074/6063#newcode53
Line 53: // timestamp.
I know it adds an extra line and some runtime cost, but why not get rid of the
return value and use Lookup() if you need to?  It seems like this is only used
for tests, so the runtime cost shouldn't be important.

http://codereview.chromium.org/118100/diff/5074/6045
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/5074/6045#newcode238
Line 238: : host_(host),
indentation is off, need another 2 spaces

http://codereview.chromium.org/118100/diff/5074/6045#newcode286
Line 286: bool was_cancelled()  const {
extra space before the const?

http://codereview.chromium.org/118100/diff/5074/6045#newcode291
Line 291: std::string host()  const {
const std::string& ?  you also have an extra space before the const.

http://codereview.chromium.org/118100/diff/5074/6045#newcode393
Line 393: HostCache::Entry* cache_entry = cache_.Lookup(hostname);
const HostCache::Entry*?

http://codereview.chromium.org/118100/diff/5074/6045#newcode473
Line 473: DCHECK(it->second.get() == job);
DCHECK_EQ

[eroman, 2009-06-11 10:37:41]

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode23
Line 23: bool HostCache::Entry::can_use(const base::TimeTicks now) const {
On 2009/06/11 08:25:15, willchan wrote:
> Do you think it'd be easier for clients if we got rid of this method?  What if
> Lookup() called this, and if it can't be used, it returns NULL?

Done.

http://codereview.chromium.org/118100/diff/5074/6065#newcode25
Line 25: // TODO(eroman): consider enabling use of negative entries.
On 2009/06/11 08:25:15, willchan wrote:
> What do you think about moving this error == OK check into HostCache::Set()
> instead?  If so, then this function might even go away entirely, and you can
add
> the simple if statement in Compact() instead.  Note that this also saves space
> in the cache.  Don't create the cache entry unless it can be used.

I would rather leave |error| in here, since I plan to use it soon.
It also helps keep some other code simple. For example, without |error|, Set()
would need to make sure to do an erase if overwriting an entry with an error
result.

(Incidentally, I added a test for the scenario mentioned above, as part of
HostCacheTest.NegativeEntry).

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
On 2009/06/11 08:25:15, willchan wrote:
> Do you think it'd be simpler if you didn't have to pin down this entry?  How
> about checking entries_size.() == max_entries_ first, and doing the compaction
> if necessary, before adding the new entry?

I considered that, but the disadvantage of doing the check before setting the
entry is you may trigger compaction too often. Specifically, when the cache is
at capacity (size == max_entries_), overwriting an existing entry should not
trigger compaction.

I reckon this scenario is worth optimizing for, since it is the expected
expected steady state if prefetching is working optimally. (So don't want to
have to traverse the entire map on each update.)

Unless you feel strongly about this, I have left as.

I experimented re-writing without the pinned parameter, but wasn't happy with
the code --- Compact() had to be redefined to prune (max_entries -1), and Set()
needed to do an extra hash lookup.

http://codereview.chromium.org/118100/diff/5074/6063
File net/base/host_cache.h (right):

http://codereview.chromium.org/118100/diff/5074/6063#newcode24
Line 24: class Entry : public base::RefCounted<Entry> {
On 2009/06/11 08:25:15, willchan wrote:
> How about making this a struct instead of a class?  It's used almost entirely
> for its data, which you've made public and don't use trailing underscores in
the
> naming anyway.  I've made a suggestion elsewhere on eliminating can_use().

Done.

http://codereview.chromium.org/118100/diff/5074/6063#newcode48
Line 48: Entry* Lookup(const std::string& hostname);
On 2009/06/11 08:25:15, willchan wrote:
> Can this be const?  Return a const Entry* instead?

Done.

http://codereview.chromium.org/118100/diff/5074/6063#newcode53
Line 53: // timestamp.
On 2009/06/11 08:25:15, willchan wrote:
> I know it adds an extra line and some runtime cost, but why not get rid of the
> return value and use Lookup() if you need to?  It seems like this is only used
> for tests, so the runtime cost shouldn't be important.

Done.

http://codereview.chromium.org/118100/diff/5074/6045
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/118100/diff/5074/6045#newcode238
Line 238: : host_(host),
On 2009/06/11 08:25:15, willchan wrote:
> indentation is off, need another 2 spaces

Done.

http://codereview.chromium.org/118100/diff/5074/6045#newcode286
Line 286: bool was_cancelled()  const {
On 2009/06/11 08:25:15, willchan wrote:
> extra space before the const?

Done.

http://codereview.chromium.org/118100/diff/5074/6045#newcode291
Line 291: std::string host()  const {
On 2009/06/11 08:25:15, willchan wrote:
> const std::string& ?  you also have an extra space before the const.

Done.

http://codereview.chromium.org/118100/diff/5074/6045#newcode393
Line 393: HostCache::Entry* cache_entry = cache_.Lookup(hostname);
On 2009/06/11 08:25:15, willchan wrote:
> const HostCache::Entry*?

Done.

http://codereview.chromium.org/118100/diff/5074/6045#newcode473
Line 473: DCHECK(it->second.get() == job);
On 2009/06/11 08:25:15, willchan wrote:
> DCHECK_EQ

Done.

[willchan no longer on Chromium, 2009-06-11 20:03:03]

I'll take a look at the tests and the rest of the code after lunch.

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode25
Line 25: // TODO(eroman): consider enabling use of negative entries.
On 2009/06/11 10:37:41, eroman wrote:
> On 2009/06/11 08:25:15, willchan wrote:
> > What do you think about moving this error == OK check into HostCache::Set()
> > instead?  If so, then this function might even go away entirely, and you can
> add
> > the simple if statement in Compact() instead.  Note that this also saves
space
> > in the cache.  Don't create the cache entry unless it can be used.
> 
> I would rather leave |error| in here, since I plan to use it soon.
> It also helps keep some other code simple. For example, without |error|, Set()
> would need to make sure to do an erase if overwriting an entry with an error
> result.

Interesting, I had not thought about this case.  Good point.  In practice, it's
probably not a concern since I'm not sure we'd ever call Set() unless the entry
were expired anyway.  But perhaps we have some situation where we want to force
the cache write.  Fair enough, this code is fine.

> 
> (Incidentally, I added a test for the scenario mentioned above, as part of
> HostCacheTest.NegativeEntry).

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
On 2009/06/11 10:37:41, eroman wrote:
> On 2009/06/11 08:25:15, willchan wrote:
> > Do you think it'd be simpler if you didn't have to pin down this entry?  How
> > about checking entries_size.() == max_entries_ first, and doing the
compaction
> > if necessary, before adding the new entry?
> 
> I considered that, but the disadvantage of doing the check before setting the
> entry is you may trigger compaction too often. Specifically, when the cache is
> at capacity (size == max_entries_), overwriting an existing entry should not
> trigger compaction.

Can you explain how this code path can rewrite an existing entry?  This is
within |if (!entry.get())|, so it must create a new entry, right?

> 
> I reckon this scenario is worth optimizing for, since it is the expected
> expected steady state if prefetching is working optimally. (So don't want to
> have to traverse the entire map on each update.)

Agreed.

> 
> Unless you feel strongly about this, I have left as.

I think I'm misunderstanding something, so I'll hold off on having a strong
opinion.

> 
> I experimented re-writing without the pinned parameter, but wasn't happy with
> the code --- Compact() had to be redefined to prune (max_entries -1), and
Set()
> needed to do an extra hash lookup.

I'm not sure about the first point here.  I was imagining:

scoped_refptr<Entry>& entry = entries_[hostname];
if (!entry.get()) {
  // Entry does not exist.  Means we have to add a new entry.  Check if we need
to compact.
  if (entries_.size() == max_entries_) {
    Compact(now);
    Entry* new_entry = new Entry(error, addrlist, expiration);
    entries_[hostname] = new_entry;
    return new_entry;
  } else {
    entry = new Entry(error, addrlist, expiration);
  }
} else {
  entry->error = error;
  entry->addrlist = addrlist;
  entry->expiration = expiration;
}

I don't think Compact() needs to only prune (max_entries - 1), it can try to
prune them all.  I concede that in the max_entries == entries_.size() case,
where we do compaction, there is an extra hash lookup.  On the other hand, in
compaction, you don't have to do an if check against the pinned entry.  IIRC,
you said the cache size might be something like 500 entries.  This means we're
trading off 500-502 branches for a single hash.  I'm not necessarily convinced
the hash cost is more than all the extra branches.

[eroman, 2009-06-11 20:19:23]

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
On 2009/06/11 20:03:03, willchan wrote:
> On 2009/06/11 10:37:41, eroman wrote:
> > On 2009/06/11 08:25:15, willchan wrote:
> > > Do you think it'd be simpler if you didn't have to pin down this entry? 
How
> > > about checking entries_size.() == max_entries_ first, and doing the
> compaction
> > > if necessary, before adding the new entry?
> > 
> > I considered that, but the disadvantage of doing the check before setting
the
> > entry is you may trigger compaction too often. Specifically, when the cache
is
> > at capacity (size == max_entries_), overwriting an existing entry should not
> > trigger compaction.
> 
> Can you explain how this code path can rewrite an existing entry?  This is
> within |if (!entry.get())|, so it must create a new entry, right?
> 
> > 
> > I reckon this scenario is worth optimizing for, since it is the expected
> > expected steady state if prefetching is working optimally. (So don't want to
> > have to traverse the entire map on each update.)
> 
> Agreed.
> 
> > 
> > Unless you feel strongly about this, I have left as.
> 
> I think I'm misunderstanding something, so I'll hold off on having a strong
> opinion.
> 
> > 
> > I experimented re-writing without the pinned parameter, but wasn't happy
with
> > the code --- Compact() had to be redefined to prune (max_entries -1), and
> Set()
> > needed to do an extra hash lookup.
> 
> I'm not sure about the first point here.  I was imagining:
> 
> scoped_refptr<Entry>& entry = entries_[hostname];
> if (!entry.get()) {
>   // Entry does not exist.  Means we have to add a new entry.  Check if we
need
> to compact.
>   if (entries_.size() == max_entries_) {
>     Compact(now);
>     Entry* new_entry = new Entry(error, addrlist, expiration);
>     entries_[hostname] = new_entry;
>     return new_entry;
>   } else {
>     entry = new Entry(error, addrlist, expiration);
>   }
> } else {
>   entry->error = error;
>   entry->addrlist = addrlist;
>   entry->expiration = expiration;
> }
> 
> I don't think Compact() needs to only prune (max_entries - 1), it can try to
> prune them all.  I concede that in the max_entries == entries_.size() case,
> where we do compaction, there is an extra hash lookup.  On the other hand, in
> compaction, you don't have to do an if check against the pinned entry.  IIRC,
> you said the cache size might be something like 500 entries.  This means we're
> trading off 500-502 branches for a single hash.  I'm not necessarily convinced
> the hash cost is more than all the extra branches.

Lets just talk about this in person; we are spending far too much time on a
minor nit.

[eroman, 2009-06-11 20:30:15]

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
> I don't think Compact() needs to only prune (max_entries - 1), it can try to
> prune them all.  I concede that in the max_entries == entries_.size() case,
> where we do compaction, there is an extra hash lookup.

Well, you need to do one or the other - either pin an entry, or prune
(max_entries - 1).

In your pseudo-code
  scoped_refptr<Entry>& entry = entries_[hostname];

will insert an entry if one didn't exist.

So now when Compact() runs there will be a NULL entry. Hence it needs to exclude
NULL entries during its iteration (pinning).

On the other hand, if you change to doing a non-mutating lookup it would look
something like:

if (!ContainsKey(entries_, hostname) && entries_.size() + 1 > max_entries_) {
  Compact(now);
}

Here obviously you need to prune to (max_entries_ - 1).

[willchan no longer on Chromium, 2009-06-11 20:50:39]

http://codereview.chromium.org/118100/diff/5074/6065
File net/base/host_cache.cc (right):

http://codereview.chromium.org/118100/diff/5074/6065#newcode62
Line 62: entry = new Entry(error, addrlist, expiration);
On 2009/06/11 20:30:15, eroman wrote:
> > I don't think Compact() needs to only prune (max_entries - 1), it can try to
> > prune them all.  I concede that in the max_entries == entries_.size() case,
> > where we do compaction, there is an extra hash lookup.
> 
> Well, you need to do one or the other - either pin an entry, or prune
> (max_entries - 1).
> 
> In your pseudo-code
>   scoped_refptr<Entry>& entry = entries_[hostname];
> 
> will insert an entry if one didn't exist.
> 
> So now when Compact() runs there will be a NULL entry. Hence it needs to
exclude
> NULL entries during its iteration (pinning).

Ah, you're right.  This makes sense now.

> 
> On the other hand, if you change to doing a non-mutating lookup it would look
> something like:
> 
> if (!ContainsKey(entries_, hostname) && entries_.size() + 1 > max_entries_) {
>   Compact(now);
> }
> 
> Here obviously you need to prune to (max_entries_ - 1).

Ok, you've convinced me!  This code looks fine.  I'll do a pass on the rest
shortly.

[willchan no longer on Chromium, 2009-06-11 21:07:29]

lgtm

http://codereview.chromium.org/118100/diff/5169/5185
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/118100/diff/5169/5185#newcode19
Line 19: FtpNetworkLayer(HostResolver* host_resolver);
explicit

http://codereview.chromium.org/118100/diff/5169/5181
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/118100/diff/5169/5181#newcode94
Line 94: DCHECK(MessageLoop::current() == host_resolver_loop_);
DCHECK_EQ

http://codereview.chromium.org/118100/diff/5169/5181#newcode100
Line 100: DCHECK(MessageLoop::current() == host_resolver_loop_);
DCHECK_EQ

[eroman, 2009-06-11 23:32:05]

Thanks for the review Will!

I have synced up to ToT, which allowed me to remove the TODO about hooking up
DNS prefetcher.

Also fixed a bug in SyncHostResolverBridge (unit test forthcoming).

I will do a bit more verification, and then try to land this sucker.
(I fear there may be some small dependency somewhere which I missed and will
cause compile break for!)

http://codereview.chromium.org/118100/diff/5169/5185
File net/ftp/ftp_network_layer.h (right):

http://codereview.chromium.org/118100/diff/5169/5185#newcode19
Line 19: FtpNetworkLayer(HostResolver* host_resolver);
On 2009/06/11 21:07:29, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/118100/diff/5169/5181
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/118100/diff/5169/5181#newcode94
Line 94: DCHECK(MessageLoop::current() == host_resolver_loop_);
On 2009/06/11 21:07:29, willchan wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/118100/diff/5169/5181#newcode100
Line 100: DCHECK(MessageLoop::current() == host_resolver_loop_);
On 2009/06/11 21:07:29, willchan wrote:
> DCHECK_EQ

Done.

[wtc, 2009-06-12 18:40:04]

http://codereview.chromium.org/118100/diff/9074/8066
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/9074/8066#newcode24
Line 24: void Copy(const struct addrinfo* head);
Eric,

In the most common case, we connect to only one or
two ports (80 or 443) on each host.  So we can get
rid of the Copy method and the changes to the nested
struct Data by making a separate getaddrinfo() call
for a different port number.  This should simplify
the code.

What do you think?

[eroman, 2009-06-12 20:15:21]

http://codereview.chromium.org/118100/diff/9074/8066
File net/base/address_list.h (right):

http://codereview.chromium.org/118100/diff/9074/8066#newcode24
Line 24: void Copy(const struct addrinfo* head);
On 2009/06/12 18:40:04, wtc wrote:
> Eric,
> 
> In the most common case, we connect to only one or
> two ports (80 or 443) on each host.  So we can get
> rid of the Copy method and the changes to the nested
> struct Data by making a separate getaddrinfo() call
> for a different port number.  This should simplify
> the code.
> 
> What do you think?

That seems really weird to me.

- We would be doing different lookups for the same host.
- The HostCache would need to be keyed by (hostname, port) rather than just
(hostname)
- Less reproducible results for user -- changing port might yield different
address!

Complexity-wise Copy() is only used in one place, so I can't imagine making
duplicate calls for the same host would simplify code any....