When measuring waste in the DNS resolver related to AF_UNSPEC lookups, also

net/base/host_resolver_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
@@ skipping 1641 matching lines @@
     RecordTotalTime(HaveDnsConfig(), info.is_speculative(), base::TimeDelta());
     return rv;
   }
 
   // Next we need to attach our request to a "job". This job is responsible for
   // calling "getaddrinfo(hostname)" on a worker thread.
 
   JobMap::iterator jobit = jobs_.find(key);
   Job* job;
   if (jobit == jobs_.end()) {
+    // If we couldn't find the desired address family, check to see if the
+    // other family is in the cache or another job, which indicates waste,
+    // and we should fix crbug.com/139811.
+    {
+      bool ipv4 = key.address_family == ADDRESS_FAMILY_IPV4;
+      Key other_family_key = key;
+      other_family_key.address_family = ipv4 ?
+          ADDRESS_FAMILY_UNSPECIFIED : ADDRESS_FAMILY_IPV4;
+      bool found_other_family_cache = false, found_other_family_job = false;

[szym, 2012/08/14 22:05:03]

nit: Although I can't find anything explicit in the code style, I suggest one
variable per declaration.

+      if (default_address_family_ == ADDRESS_FAMILY_UNSPECIFIED) {
+        found_other_family_cache =
+            cache_->Lookup(other_family_key, base::TimeTicks::Now()) != NULL;
+        if (!found_other_family_cache)
+          found_other_family_job = jobs_.find(other_family_key) != jobs_.end();

[szym, 2012/08/14 22:05:03]

I suggest |jobs_.count(other_family_key) > 0|

+      }
+      enum {  // Used in HISTOGRAM_ENUMERATION.

[szym, 2012/08/14 22:05:03]

nit: UMA_HISTOGRAM_ENUMERATION

+        AF_WASTE_IPV4_ONLY,
+        AF_WASTE_CACHE_IPV4,
+        AF_WASTE_CACHE_UNSPEC,
+        AF_WASTE_JOB_IPV4,
+        AF_WASTE_JOB_UNSPEC,
+        AF_WASTE_MISS_IPV4,

[szym, 2012/08/14 22:05:03]

How about AF_WASTE_NONE_* to indicate that there is no waste in this case?

+        AF_WASTE_MISS_UNSPEC,
+        AF_WASTE_MAX,  // Bounding value.
+      } category = AF_WASTE_MAX;
+      if (default_address_family_ != ADDRESS_FAMILY_UNSPECIFIED) {
+        category = AF_WASTE_IPV4_ONLY;
+      } else if (found_other_family_cache) {
+        category = ipv4 ? AF_WASTE_CACHE_IPV4 : AF_WASTE_CACHE_UNSPEC;
+      } else if (found_other_family_job) {
+        category = ipv4 ? AF_WASTE_JOB_IPV4 : AF_WASTE_JOB_UNSPEC;
+      } else {
+        category = ipv4 ? AF_WASTE_MISS_IPV4 : AF_WASTE_MISS_UNSPEC;
+      }
+      UMA_HISTOGRAM_ENUMERATION("DNS.ResolveUnspecWasteCategory", category,

[szym, 2012/08/14 22:05:03]

nit: I don't think you need "Category". "Waste" seems sufficiently specific.

+                                AF_WASTE_MAX);
+    }
+
     // Create new Job.
     job = new Job(this, key, info.priority(), request_net_log);
     job->Schedule();
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
       Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());
       DCHECK(evicted);
       evicted->OnEvicted();  // Deletes |evicted|.
       if (evicted == job) {
@@ skipping 141 matching lines @@
 
 bool HostResolverImpl::ServeFromCache(const Key& key,
                                       const RequestInfo& info,
                                       int* net_error,
                                       AddressList* addresses) {
   DCHECK(addresses);
   DCHECK(net_error);
   if (!info.allow_cached_response() || !cache_.get())
     return false;
 
-  base::TimeTicks current_time = base::TimeTicks::Now();
-  const HostCache::Entry* cache_entry = cache_->Lookup(key, current_time);
-
-  {
-    bool found = cache_entry != NULL;
-    bool ipv4 = key.address_family == ADDRESS_FAMILY_IPV4;
-    // If we couldn't find the desired address family, check to see if the
-    // other family is in the cache, which indicates waste, and we should fix
-    // crbug.com/139811.
-    bool found_other_family = false;
-    if (!found && default_address_family_ == ADDRESS_FAMILY_UNSPECIFIED) {
-      Key other_family_key = key;
-      other_family_key.address_family = ipv4 ?
-          ADDRESS_FAMILY_UNSPECIFIED : ADDRESS_FAMILY_IPV4;
-      found_other_family =
-          cache_->Lookup(other_family_key, current_time) != NULL;
-    }
-    enum {  // Used in HISTOGRAM_ENUMERATION.
-      CACHE_IPV4_ONLY_FOUND,
-      CACHE_IPV4_ONLY_MISS,
-      CACHE_FOUND_IPV4,
-      CACHE_FOUND_UNSPEC,
-      CACHE_WASTE_IPV4,
-      CACHE_WASTE_UNSPEC,
-      CACHE_MISS_IPV4,
-      CACHE_MISS_UNSPEC,
-      CACHE_MAX,  // Bounding value.
-    } category = CACHE_MAX;
-    if (default_address_family_ != ADDRESS_FAMILY_UNSPECIFIED) {
-      category = found ? CACHE_IPV4_ONLY_FOUND : CACHE_IPV4_ONLY_MISS;
-    } else if (found) {
-      category = ipv4 ? CACHE_FOUND_IPV4 : CACHE_FOUND_UNSPEC;
-    } else if (found_other_family) {
-      category = ipv4 ? CACHE_WASTE_IPV4 : CACHE_WASTE_UNSPEC;
-    } else {
-      category = ipv4 ? CACHE_MISS_IPV4 : CACHE_MISS_UNSPEC;
-    }
-    UMA_HISTOGRAM_ENUMERATION("DNS.ResolveCacheCategory", category, CACHE_MAX);
-  }
-
+  const HostCache::Entry* cache_entry = cache_->Lookup(
+      key, base::TimeTicks::Now());
   if (!cache_entry)
     return false;
 
   *net_error = cache_entry->error;
   if (*net_error == OK) {
     *addresses = cache_entry->addrlist;
     EnsurePortOnAddressList(info.port(), addresses);
   }
   return true;
 }
@@ skipping 178 matching lines @@
   // |this| may be deleted inside AbortAllInProgressJobs().
   if (self)
     TryServingAllJobsFromHosts();
 }
 
 bool HostResolverImpl::HaveDnsConfig() const {
   return (dns_client_.get() != NULL) && (dns_client_->GetConfig() != NULL);
 }
 
 }  // namespace net