Refactor DNS A/B experient, and add test of congestion time limits

I added another option in the DNS experiment: user 2 seconds or 500ms
for the congestion limit (that causes pre-resolutions to be discarded
from the queue.

The code in browser_main.cc was getting toooo large, so I pulled all
the experiment code into dns_global.cc.

BUG=15479

r=eroman
Review URL: http://codereview.chromium.org/147215

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@19464 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 126 insertions, 78 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index df94b45..91bc5e4 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -635,53 +635,8 @@ int BrowserMain(const MainFunctionParams& parameters) {
   net::EnsureWinsockInit();
 #endif  // defined(OS_WIN)
 
-  // Set up a field trial to see what disabling DNS pre-resolution does to
-  // latency of network transactions.
-  FieldTrial::Probability kDIVISOR = 100;
-  FieldTrial::Probability k_PROBABILITY_PER_GROUP = 10;  // 10% probability.
-  // For options we don't (currently) wish to test, we use zero probability.
-  FieldTrial::Probability k_PROBABILITY_DISABLED = 0;
-  scoped_refptr<FieldTrial> dns_trial = new FieldTrial("DnsImpact", kDIVISOR);
-
-  // First option is to disable prefetching completele.
-  dns_trial->AppendGroup("_disabled_prefetch", k_PROBABILITY_PER_GROUP);
-  // Second option is to set parallel prefetch limit to 4 instead of default 8.
-  int parallel_4_prefetch = dns_trial->AppendGroup("_parallel_4_prefetch",
-                                                   k_PROBABILITY_PER_GROUP);
-
-  // Don't discard names (erase these lines) yet, as we may use them, and we
-  // have histogram data named for these options.
-
-  // Next two options relate to the number of parallel http connections that can
-  // be made to a single host.  The default is currently 6, and we wanted to see
-  // what would happen when we restricted this to 4.  (Historically the limit
-  // was a mere 2, but several browsers including Chromium increased it to 6
-  // recently).
-  int disabled_plus_4_connections = dns_trial->AppendGroup(
-      "_disabled_prefetch_4_connections", k_PROBABILITY_DISABLED);
-  int enabled_plus_4_connections = dns_trial->AppendGroup(
-      "_enabled_prefetch_4_connections", k_PROBABILITY_DISABLED);
-
-  scoped_ptr<chrome_browser_net::DnsPrefetcherInit> dns_prefetch_init;
-  if (dns_trial->group() == FieldTrial::kNotParticipating ||
-    dns_trial->group() == enabled_plus_4_connections ||
-    dns_trial->group() == parallel_4_prefetch) {
-    // Initialize the DNS prefetch system
-      if (dns_trial->group() == parallel_4_prefetch)
-        dns_prefetch_init.reset(new chrome_browser_net::DnsPrefetcherInit(
-            4, user_prefs));
-      else
-        dns_prefetch_init.reset(new chrome_browser_net::DnsPrefetcherInit(
-            chrome_browser_net::DnsPrefetcherInit::kMaxConcurrentLookups,
-            user_prefs));
-    chrome_browser_net::DnsPrefetchHostNamesAtStartup(user_prefs, local_state);
-    chrome_browser_net::RestoreSubresourceReferrers(local_state);
-  }
-
-  if (dns_trial->group() == disabled_plus_4_connections ||
-      dns_trial->group() == enabled_plus_4_connections) {
-    net::HttpNetworkSession::set_max_sockets_per_group(4);
-  }
+  // Initialize and maintain DNS prefetcher module.
+  chrome_browser_net::DnsPrefetcherInit dns_prefetch(user_prefs, local_state);
 
   scoped_refptr<FieldTrial> http_prioritization_trial =
       new FieldTrial("HttpPrioritization", 100);
diff --git a/chrome/browser/net/dns_global.cc b/chrome/browser/net/dns_global.cc
index 379bf0f..68cfad3 100644
--- a/chrome/browser/net/dns_global.cc
+++ b/chrome/browser/net/dns_global.cc
@@ -24,6 +24,7 @@
 #include "chrome/common/pref_service.h"
 #include "net/base/host_resolver.h"
 
+using base::Time;
 using base::TimeDelta;
 
 namespace chrome_browser_net {
@@ -38,6 +39,9 @@ static void DnsPrefetchMotivatedList(
 // static
 const size_t DnsPrefetcherInit::kMaxConcurrentLookups = 8;
 
+// static
+const int DnsPrefetcherInit::kMaxQueueingDelayMs = 1000;
+
 // Host resolver shared by DNS prefetcher, and the main URLRequestContext.
 static net::HostResolver* global_host_resolver = NULL;
 
@@ -281,7 +285,7 @@ void PrefetchObserver::SaveStartupListAsPref(PrefService* local_state) {
   if (!startup_list)
     return;
   startup_list->Clear();
-  DCHECK(startup_list->GetSize() == 0);
+  DCHECK_EQ(0u, startup_list->GetSize());
   AutoLock auto_lock(*lock);
   for (Results::iterator it = first_resolutions->begin();
        it != first_resolutions->end();
@@ -346,7 +350,7 @@ class OffTheRecordObserver : public NotificationObserver {
           break;  // Ignore ordinary windows.
         {
           AutoLock lock(lock_);
-          DCHECK(0 < count_off_the_record_windows_);
+          DCHECK_LT(0, count_off_the_record_windows_);
           if (0 >= count_off_the_record_windows_)  // Defensive coding.
             break;
           if (--count_off_the_record_windows_)
@@ -405,7 +409,8 @@ void DnsPrefetchGetHtmlInfo(std::string* output) {
 // can ensure its deletion.
 static PrefetchObserver dns_resolution_observer;
 
-void InitDnsPrefetch(size_t max_concurrent, PrefService* user_prefs) {
+void InitDnsPrefetch(TimeDelta max_queue_delay, size_t max_concurrent,
+                     PrefService* user_prefs) {
   // Use a large shutdown time so that UI tests (that instigate lookups, and
   // then try to shutdown the browser) don't instigate the CHECK about
   // "some slaves have not finished"
@@ -416,7 +421,7 @@ void InitDnsPrefetch(size_t max_concurrent, PrefService* user_prefs) {
     // that is shared by the main URLRequestContext, and lives on the IO thread.
     dns_master = new DnsMaster(GetGlobalHostResolver(),
                                g_browser_process->io_thread()->message_loop(),
-                               max_concurrent);
+                               max_queue_delay, max_concurrent);
     dns_master->AddRef();
     // We did the initialization, so we should prime the pump, and set up
     // the DNS resolution system to run.
@@ -555,4 +560,62 @@ void TrimSubresourceReferrers() {
   dns_master->TrimReferrers();
 }
 
+//------------------------------------------------------------------------------
+// Methods for the helper class that is used to startup and teardown the whole
+// DNS prefetch system.
+
+DnsPrefetcherInit::DnsPrefetcherInit(PrefService* user_prefs,
+                                     PrefService* local_state) {
+  // Set up a field trial to see what disabling DNS pre-resolution does to
+  // latency of page loads.
+  FieldTrial::Probability kDivisor = 100;
+  // For each option (i.e., non-default), we have a fixed probability.
+  FieldTrial::Probability kProbabilityPerGroup = 10;  // 10% probability.
+
+  trial_ = new FieldTrial("DnsImpact", kDivisor);
+
+  // First option is to disable prefetching completely.
+  int disabled_prefetch = trial_->AppendGroup("_disabled_prefetch",
+                                              kProbabilityPerGroup);
+  // Set parallel prefetch limit to 4 instead of default 8.
+  int parallel_4_prefetch = trial_->AppendGroup("_parallel_4_prefetch",
+                                                kProbabilityPerGroup);
+  // Set congestion detection at 500ms, rather than the 1 second default.
+  int max_500ms_prefetch = trial_->AppendGroup("_max_500ms_prefetch_queue",
+                                               kProbabilityPerGroup);
+  // Set congestion detection at 2 seconds instead of the 1 second default.
+  int max_2s_prefetch = trial_->AppendGroup("_max_2s_prefetch_queue",
+                                            kProbabilityPerGroup);
+
+  if (trial_->group() != disabled_prefetch) {
+    // Initialize the DNS prefetch system.
+
+    size_t max_concurrent = kMaxConcurrentLookups;
+
+    int max_queueing_delay_ms = kMaxQueueingDelayMs;
+
+    if (trial_->group() == parallel_4_prefetch)
+      max_concurrent = 4;
+    else if (trial_->group() == max_500ms_prefetch)
+      max_queueing_delay_ms = 500;
+    else if (trial_->group() == max_2s_prefetch)
+      max_queueing_delay_ms = 2000;
+
+    TimeDelta max_queueing_delay(
+        TimeDelta::FromMilliseconds(max_queueing_delay_ms));
+
+    DCHECK(!dns_master);
+    InitDnsPrefetch(max_queueing_delay, max_concurrent, user_prefs);
+    DCHECK(dns_master);  // Will be checked in destructor.
+    chrome_browser_net::DnsPrefetchHostNamesAtStartup(user_prefs, local_state);
+    chrome_browser_net::RestoreSubresourceReferrers(local_state);
+  }
+}
+
+DnsPrefetcherInit::~DnsPrefetcherInit() {
+    if (dns_master)
+      FreeDnsPrefetchResources();
+  }
+
 }  // namespace chrome_browser_net
+
diff --git a/chrome/browser/net/dns_global.h b/chrome/browser/net/dns_global.h
index e27062e..aecfe8c 100644
--- a/chrome/browser/net/dns_global.h
+++ b/chrome/browser/net/dns_global.h
@@ -11,10 +11,13 @@
 #ifndef CHROME_BROWSER_NET_DNS_GLOBAL_H_
 #define CHROME_BROWSER_NET_DNS_GLOBAL_H_
 
-#include "chrome/browser/net/dns_master.h"
 
 #include <string>
 
+#include "base/field_trial.h"
+#include "base/scoped_ptr.h"
+#include "chrome/browser/net/dns_master.h"
+
 class PrefService;
 
 namespace net {
@@ -25,7 +28,8 @@ namespace chrome_browser_net {
 
 // Initialize dns prefetching subsystem. Must be called before any other
 // functions.
-void InitDnsPrefetch(size_t max_concurrent, PrefService* user_prefs);
+void InitDnsPrefetch(TimeDelta max_queue_delay, size_t max_concurrent,
+                     PrefService* user_prefs);
 
 // Cancel pending lookup requests and don't make new ones. Does nothing
 // if dns prefetching has not been initialized (to simplify its usage).
@@ -66,19 +70,23 @@ void TrimSubresourceReferrers();
 // Helper class to handle global init and shutdown.
 class DnsPrefetcherInit {
  public:
-  // Too many concurrent lookups negate benefits of prefetching by trashing the
-  // OS cache before all resource loading is complete.  This is the default.
+  // Too many concurrent lookups negate benefits of prefetching by trashing
+  // the OS cache before all resource loading is complete.
+  // This is the default.
   static const size_t kMaxConcurrentLookups;
 
-  DnsPrefetcherInit(size_t max_concurrent, PrefService* user_prefs) {
-    InitDnsPrefetch(max_concurrent, user_prefs);
-  }
+  // When prefetch requests are queued beyond some period of time, then the
+  // system is congested, and we need to clear all queued requests to get out
+  // of that state.  The following is the suggested default time limit.
+  static const int kMaxQueueingDelayMs;
 
-  ~DnsPrefetcherInit() {
-    FreeDnsPrefetchResources();
-  }
+  DnsPrefetcherInit(PrefService* user_prefs, PrefService* local_state);
+  ~DnsPrefetcherInit();
 
  private:
+  // Maintain a field trial instance when we do A/B testing.
+  scoped_refptr<FieldTrial> trial_;
+
   DISALLOW_COPY_AND_ASSIGN(DnsPrefetcherInit);
 };
 
diff --git a/chrome/browser/net/dns_master.cc b/chrome/browser/net/dns_master.cc
index 8cbae92..2cc50ff 100644
--- a/chrome/browser/net/dns_master.cc
+++ b/chrome/browser/net/dns_master.cc
@@ -20,6 +20,8 @@
 #include "net/base/host_resolver.h"
 #include "net/base/net_errors.h"
 
+using base::TimeDelta;
+
 namespace chrome_browser_net {
 
 class DnsMaster::LookupRequest {
@@ -68,10 +70,12 @@ class DnsMaster::LookupRequest {
 
 DnsMaster::DnsMaster(net::HostResolver* host_resolver,
                      MessageLoop* host_resolver_loop,
+                     TimeDelta max_queue_delay,
                      size_t max_concurrent)
   : peak_pending_lookups_(0),
     shutdown_(false),
     max_concurrent_lookups_(max_concurrent),
+    max_queue_delay_(max_queue_delay),
     host_resolver_(host_resolver),
     host_resolver_loop_(host_resolver_loop) {
 }
@@ -350,7 +354,7 @@ void DnsMaster::GetHtmlInfo(std::string* output) {
     }
     if (!it->second.was_found())
       continue;  // Still being processed.
-    if (base::TimeDelta() != it->second.benefits_remaining()) {
+    if (TimeDelta() != it->second.benefits_remaining()) {
       network_hits.push_back(it->second);  // With no benefit yet.
       continue;
     }
@@ -385,7 +389,7 @@ DnsHostInfo* DnsMaster::PreLockedResolve(
     const std::string& hostname,
     DnsHostInfo::ResolutionMotivation motivation) {
   // DCHECK(We have the lock);
-  DCHECK(0 != hostname.length());
+  DCHECK_NE(0u, hostname.length());
 
   if (shutdown_)
     return NULL;
@@ -444,9 +448,7 @@ void DnsMaster::PreLockedScheduleLookups() {
 
 bool DnsMaster::PreLockedCongestionControlPerformed(DnsHostInfo* info) {
   // Note: queue_duration is ONLY valid after we go to assigned state.
-  // TODO(jar): Tune selection of arbitrary 1 second constant.  Add plumbing so
-  // that this can be set as part of an A/B experiment.
-  if (info->queue_duration() < base::TimeDelta::FromSeconds(1))
+  if (info->queue_duration() < max_queue_delay_)
     return false;
   // We need to discard all entries in our queue, as we're keeping them waiting
   // too long.  By doing this, we'll have a chance to quickly service urgent
diff --git a/chrome/browser/net/dns_master.h b/chrome/browser/net/dns_master.h
index 04fc56f..b4af6f4 100644
--- a/chrome/browser/net/dns_master.h
+++ b/chrome/browser/net/dns_master.h
@@ -26,6 +26,8 @@
 #include "chrome/common/net/dns.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
+using base::TimeDelta;
+
 namespace net {
 class HostResolver;
 }
@@ -42,9 +44,8 @@ class DnsMaster : public base::RefCountedThreadSafe<DnsMaster> {
   // |max_concurrent| specifies how many concurrent (paralell) prefetches will
   // be performed. Host lookups will be issued on the |host_resolver_loop|
   // thread, using the |host_resolver| instance.
-  DnsMaster(net::HostResolver* host_resolver,
-            MessageLoop* host_resolver_loop,
-            size_t max_concurrent);
+  DnsMaster(net::HostResolver* host_resolver, MessageLoop* host_resolver_loop,
+            TimeDelta max_queue_delay_ms, size_t max_concurrent);
   ~DnsMaster();
 
   // Cancel pending requests and prevent new ones from being made.
@@ -227,6 +228,10 @@ class DnsMaster : public base::RefCountedThreadSafe<DnsMaster> {
   // The number of concurrent lookups currently allowed.
   const size_t max_concurrent_lookups_;
 
+  // The maximum queueing delay that is acceptable before we enter congestion
+  // reduction mode, and discard all queued (but not yet assigned) resolutions.
+  const TimeDelta max_queue_delay_;
+
   // The host resovler we warm DNS entries for. The resolver (which is not
   // thread safe) should be accessed only on |host_resolver_loop_|.
   scoped_refptr<net::HostResolver> host_resolver_;
diff --git a/chrome/browser/net/dns_master_unittest.cc b/chrome/browser/net/dns_master_unittest.cc
index da720fe..921b43b 100644
--- a/chrome/browser/net/dns_master_unittest.cc
+++ b/chrome/browser/net/dns_master_unittest.cc
@@ -60,6 +60,8 @@ class DnsMasterTest : public testing::Test {
  public:
   DnsMasterTest()
       : mapper_(new net::RuleBasedHostMapper()),
+        default_max_queueing_delay_(TimeDelta::FromMilliseconds(
+            DnsPrefetcherInit::kMaxQueueingDelayMs)),
         scoped_mapper_(mapper_.get()) {
   }
 
@@ -84,6 +86,10 @@ class DnsMasterTest : public testing::Test {
 
   scoped_refptr<net::RuleBasedHostMapper> mapper_;
 
+  // Shorthand to access TimeDelta of DnsPrefetcherInit::kMaxQueueingDelayMs.
+  // (It would be a static constant... except style rules preclude that :-/ ).
+  const TimeDelta default_max_queueing_delay_;
+
  private:
   MessageLoop loop;
   net::ScopedHostMapper scoped_mapper_;
@@ -163,13 +169,15 @@ TEST_F(DnsMasterTest, OsCachesLookupsTest) {
 
 TEST_F(DnsMasterTest, StartupShutdownTest) {
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
   testing_master->Shutdown();
 }
 
 TEST_F(DnsMasterTest, BenefitLookupTest) {
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
 
   std::string goog("www.google.com"),
     goog2("gmail.google.com.com"),
@@ -232,7 +240,8 @@ TEST_F(DnsMasterTest, ShutdownWhenResolutionIsPendingTest) {
   net::ScopedHostMapper scoped_mapper(mapper.get());
 
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
 
   std::string localhost("127.0.0.1");
   NameList names;
@@ -255,7 +264,8 @@ TEST_F(DnsMasterTest, ShutdownWhenResolutionIsPendingTest) {
 
 TEST_F(DnsMasterTest, SingleLookupTest) {
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
 
   std::string goog("www.google.com");
 
@@ -284,7 +294,8 @@ TEST_F(DnsMasterTest, ConcurrentLookupTest) {
   mapper_->AddSimulatedFailure("*.notfound");
 
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
 
   std::string goog("www.google.com"),
     goog2("gmail.google.com.com"),
@@ -338,7 +349,8 @@ TEST_F(DnsMasterTest, DISABLED_MassiveConcurrentLookupTest) {
   mapper_->AddSimulatedFailure("*.notfound");
 
   scoped_refptr<DnsMaster> testing_master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
 
   NameList names;
   for (int i = 0; i < 100; i++)
@@ -442,7 +454,8 @@ int GetLatencyFromSerialization(const std::string& motivation,
 // Make sure nil referral lists really have no entries, and no latency listed.
 TEST_F(DnsMasterTest, ReferrerSerializationNilTest) {
   scoped_refptr<DnsMaster> master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
   ListValue referral_list;
   master->SerializeReferrers(&referral_list);
   EXPECT_EQ(0U, referral_list.GetSize());
@@ -457,7 +470,8 @@ TEST_F(DnsMasterTest, ReferrerSerializationNilTest) {
 // serialization without being changed.
 TEST_F(DnsMasterTest, ReferrerSerializationSingleReferrerTest) {
   scoped_refptr<DnsMaster> master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
   std::string motivation_hostname = "www.google.com";
   std::string subresource_hostname = "icons.google.com";
   const int kLatency = 3;
@@ -481,7 +495,8 @@ TEST_F(DnsMasterTest, ReferrerSerializationSingleReferrerTest) {
 // Make sure the Trim() functionality works as expected.
 TEST_F(DnsMasterTest, ReferrerSerializationTrimTest) {
   scoped_refptr<DnsMaster> master = new DnsMaster(new net::HostResolver,
-      MessageLoop::current(), DnsPrefetcherInit::kMaxConcurrentLookups);
+      MessageLoop::current(), default_max_queueing_delay_,
+      DnsPrefetcherInit::kMaxConcurrentLookups);
   std::string motivation_hostname = "www.google.com";
   std::string icon_subresource_hostname = "icons.google.com";
   std::string img_subresource_hostname = "img.google.com";