Clean up dns prefetch code, and also port it.

chrome/browser/net/dns_master.h

Index: chrome/browser/net/dns_master.h
diff --git a/chrome/browser/net/dns_master.h b/chrome/browser/net/dns_master.h
index 72116cdac48b7de71dfa0ae269b677839f12d250..8018e26857dba21685ca9b063775493778e800f2 100644
--- a/chrome/browser/net/dns_master.h
+++ b/chrome/browser/net/dns_master.h
@@ -3,59 +3,45 @@
 // found in the LICENSE file.
 
 // A DnsMaster object is instantiated once in the browser
-// process, and delivers DNS prefetch assignments (hostnames)
-// to any of several DnsSlave objects.
+// process, and manages asynchronous resolution of DNS hostnames.
 // Most hostname lists are sent out by renderer processes, and
 // involve lists of hostnames that *might* be used in the near
 // future by the browsing user.  The goal of this class is to
 // cause the underlying DNS structure to lookup a hostname before
 // it is really needed, and hence reduce latency in the standard
-// lookup paths.  Since some DNS lookups may take a LONG time, we
-// use several DnsSlave threads to concurrently perform the
-// lookups.
+// lookup paths.
 
 #ifndef CHROME_BROWSER_NET_DNS_MASTER_H_
 #define CHROME_BROWSER_NET_DNS_MASTER_H_
 
 #include <map>
 #include <queue>
+#include <set>
 #include <string>
 
-#include "base/condition_variable.h"
-#include "base/scoped_ptr.h"
-#include "base/values.h"
+#include "base/lock.h"
 #include "chrome/browser/net/dns_host_info.h"
 #include "chrome/browser/net/referrer.h"
 #include "chrome/common/net/dns.h"
 #include "googleurl/src/url_canon.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 namespace chrome_browser_net {
 
-class DnsSlave;
-
 typedef chrome_common_net::NameList NameList;
 typedef std::map<std::string, DnsHostInfo> Results;
 
 class DnsMaster {
  public:
-  // The number of slave processes that will do DNS prefetching
-  static const size_t kSlaveCountMax = 8;
-
-  explicit DnsMaster(base::TimeDelta shutdown_wait_time);
+  // Too many concurrent lookups negate benefits of prefetching
+  // by trashing the OS cache.
+  static const size_t kMaxConcurrentLookups;
 
-  ~DnsMaster() {
-    if (!shutdown_)
-      ShutdownSlaves();  // Ensure we did our cleanup.
-  }
+  DnsMaster();
+  ~DnsMaster();
 
-  // ShutdownSlaves() gets all spawned threads to terminate, closes
-  // their handles, and deletes their DnsSlave instances.
-  // Return value of true means all operations succeeded.
-  // Return value of false means that the threads wouldn't terminate,
-  // and that resources may leak.  If this returns false, it is best
-  // to NOT delete this DnsMaster, as slave threads may still call into
-  // this object.
-  bool ShutdownSlaves();
+  // Cancel pending requests and prevent new ones from being made.
+  void Shutdown();
 
   // In some circumstances, for privacy reasons, all results should be
   // discarded.  This method gracefully handles that activity.
@@ -64,7 +50,7 @@ class DnsMaster {
   // (cache hits etc.).
   void DiscardAllResults();
 
-  // Add hostname(s) to the queue for processing by slaves
+  // Add hostname(s) to the queue for processing.
   void ResolveList(const NameList& hostnames,
                    DnsHostInfo::ResolutionMotivation motivation);
   void Resolve(const std::string& hostname,
@@ -89,36 +75,6 @@ class DnsMaster {
   // domains.
   void GetHtmlInfo(std::string* output);
 
-  // For testing only...
-  // Currently testing only provides a crude measure of success.
-  bool WasFound(const std::string& hostname) {
-    AutoLock auto_lock(lock_);
-    return (results_.find(hostname) != results_.end()) &&
-            results_[hostname].was_found();
-  }
-
-  // Accessor methods, used mostly for testing.
-  // Both functions return DnsHostInfo::kNullDuration if name was not yet
-  // processed enough.
-  base::TimeDelta GetResolutionDuration(const std::string hostname) {
-    AutoLock auto_lock(lock_);
-    if (results_.find(hostname) == results_.end())
-      return DnsHostInfo::kNullDuration;
-    return results_[hostname].resolve_duration();
-  }
-
-  base::TimeDelta GetQueueDuration(const std::string hostname) {
-    AutoLock auto_lock(lock_);
-    if (results_.find(hostname) == results_.end())
-      return DnsHostInfo::kNullDuration;
-    return results_[hostname].queue_duration();
-  }
-
-  size_t running_slave_count() {
-    AutoLock auto_lock(lock_);
-    return running_slave_count_;
-  }
-
   // Discard any referrer for which all the suggested host names are currently
   // annotated with no user latency reduction.  Also scale down (diminish) the
   // total benefit of those that did help, so that their reported contribution
@@ -135,42 +91,58 @@ class DnsMaster {
   // values into the current referrer list.
   void DeserializeReferrers(const ListValue& referral_list);
 
-  //----------------------------------------------------------------------------
-  // Methods below this line should only be called by slave processes.
-
-  // GetNextAssignment() gets the next hostname from queue for processing
-  // It is not meant to be public, and should only be used by the slave.
-  // GetNextAssignment() waits on a condition variable if there are no more
-  // names in queue.
-  // Return false if slave thread should terminate.
-  // Return true if slave thread should process the value.
-  bool GetNextAssignment(std::string* hostname);
-
-  // Access methods for use by slave threads to callback with state updates.
-  void SetFoundState(const std::string hostname);
-  void SetNoSuchNameState(const std::string hostname);
+ private:
+  FRIEND_TEST(DnsMasterTest, BenefitLookupTest);
+  FRIEND_TEST(DnsMasterTest, ShutdownWhenResolutionIsPendingTest);
+  FRIEND_TEST(DnsMasterTest, SingleLookupTest);
+  FRIEND_TEST(DnsMasterTest, ConcurrentLookupTest);
+  FRIEND_TEST(DnsMasterTest, MassiveConcurrentLookupTest);
+  friend class WaitForResolutionHelper;  // For testing.
 
-  // Notification during ShutdownSlaves.
-  void SetSlaveHasTerminated(int slave_index);
+  class LookupRequest;
 
- private:
   // A map that is keyed with the hostnames that we've learned were the cause
   // of loading additional hostnames.  The list of additional hostnames in held
   // in a Referrer instance, which is found in this type.
   typedef std::map<std::string, Referrer> Referrers;
 
+  // Only for testing. Returns true if hostname has been successfully resolved
+  // (name found).
+  bool WasFound(const std::string& hostname) {
+    AutoLock auto_lock(lock_);
+    return (results_.find(hostname) != results_.end()) &&
+            results_[hostname].was_found();
+  }
+
+  // Only for testing. Return how long was the resolution
+  // or DnsHostInfo::kNullDuration if it hasn't been resolved yet.
+  base::TimeDelta GetResolutionDuration(const std::string& hostname) {
+    AutoLock auto_lock(lock_);
+    if (results_.find(hostname) == results_.end())
+      return DnsHostInfo::kNullDuration;
+    return results_[hostname].resolve_duration();
+  }
+
+  // Only for testing;
+  size_t peak_pending_lookups() const { return peak_pending_lookups_; }
+
+  // Access method for use by lookup request to pass resolution result.
+  void OnLookupFinished(LookupRequest* request,
+                        const std::string& hostname, bool found);
+
   // "PreLocked" means that the caller has already Acquired lock_ in the
   // following method names.
   // Queue hostname for resolution.  If queueing was done, return the pointer
   // to the queued instance, otherwise return NULL.
   DnsHostInfo* PreLockedResolve(const std::string& hostname,
                                 DnsHostInfo::ResolutionMotivation motivation);
-  bool PreLockedCreateNewSlaveIfNeeded();  // Lazy slave processes creation.
 
-  // Number of slave processes started early (to help with startup prefetch).
-  static const size_t kSlaveCountMin = 4;
+  // Take lookup requests from name_buffer_ and tell HostResolver
+  // to look them up asynchronously, provided we don't exceed
+  // concurrent resolution limit.
+  void PreLockedScheduleLookups();
 
-  // Synchronize access to results_, referrers_, and slave control data.
+  // Synchronize access to variables listed below.

[darin (slow to review), 2009/02/18 21:49:17]

looks like a good comment to me.

   Lock lock_;
 
   // name_buffer_ holds a list of names we need to look up.
@@ -184,30 +156,13 @@ class DnsMaster {
   // pre-resolve when there is a navigation to the orginial hostname.
   Referrers referrers_;
 
-  // Signaling slaves to process elements in the queue, or to terminate,
-  // is done using ConditionVariables.
-  ConditionVariable slaves_have_work_;
-
-  size_t slave_count_;  // Count of slave processes started.
-  size_t running_slave_count_;  // Count of slaves process still running.
-
-  // TODO(jrg): wait for CL 15076 from _ph to come in which resolves
-  // this.  In the short term this file is hacked to be happy when
-  // included in render_process.h.
-#if defined(OS_WIN)
-  // The following arrays are only initialized as
-  // slave_count_ grows (up to the indicated max).
-  DWORD thread_ids_[kSlaveCountMax];
-  HANDLE thread_handles_[kSlaveCountMax];
-  DnsSlave* slaves_[kSlaveCountMax];
-#endif
-
-  // shutdown_ is set to tell the slaves to terminate.
-  bool shutdown_;
+  std::set<LookupRequest*> pending_lookups_;
 
-  // The following is the maximum time the ShutdownSlaves method
-  // will wait for all the slave processes to terminate.
-  const base::TimeDelta kShutdownWaitTime_;
+  // For testing, to verify that we don't exceed the limit.
+  size_t peak_pending_lookups_;
+
+  // When true, we don't make new lookup requests.
+  bool shutdown_;
 
   // A list of successful events resulting from pre-fetching.
   DnsHostInfo::DnsInfoTable cache_hits_;