Reduce "false positive" in preconnect rates.

Histograms suggest that all too often, we create a preconnection
for an omnibox entry (for a search URL), and then the 
connection goes unused.  This probably happens when
the first few letters of an URL are typed, but are
misconstrued to be a search URL.  I added code that
requires a sequence of consecutive assertions by the
omnibox that the URL is preconnectable (i.e., is a
search URL).  This will give me something to tune, but
this CL pulls a number out of teh air, in hopes of
getting some baseline.

I also noticed that the unused connections courtesy 
of speculative subresource preconnection was higher
than expected, and the number of cases where we
didn't bother to even do a pre-resolut1ion for
a subresource was higher than expected.  It used
to be that I did sub-resource pre-resolution for
all 8 learned speculative names.  Now that we
have preconnection as an alternative, I extended
the count to 10 names learned, but put in a
threshold for expected connections.  I adjusted
the thresholds to increase the amount of
pre-resolution, and decrease the amount of
preconection.  I really need an A/B test
to optimize these, but initially I wanted
to get a feel for what happens with different
limits.

BUG=42694
r=mbelshe
Review URL: http://codereview.chromium.org/3126012

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

4 files changed, 190 insertions, 111 deletions

diff --git a/chrome/browser/net/predictor.cc b/chrome/browser/net/predictor.cc
index c698bbc..1602cc1 100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -28,11 +28,11 @@ using base::TimeDelta;
 namespace chrome_browser_net {
 
 // static
-const double Predictor::kPreconnectWorthyExpectedValue = 0.7;
+const double Predictor::kPreconnectWorthyExpectedValue = 0.8;
 // static
-const double Predictor::kDNSPreresolutionWorthyExpectedValue = 0.2;
+const double Predictor::kDNSPreresolutionWorthyExpectedValue = 0.1;
 // static
-const double Predictor::kPersistWorthyExpectedValue = 0.1;
+const double Predictor::kPersistWorthyExpectedValue = 0.05;
 
 
 class Predictor::LookupRequest {
@@ -89,7 +89,8 @@ Predictor::Predictor(net::HostResolver* host_resolver,
       max_concurrent_dns_lookups_(max_concurrent),
       max_dns_queue_delay_(max_dns_queue_delay),
       host_resolver_(host_resolver),
-      preconnect_enabled_(preconnect_enabled) {
+      preconnect_enabled_(preconnect_enabled),
+      consecutive_omnibox_preconnect_count_(0) {
   Referrer::SetUsePreconnectValuations(preconnect_enabled);
 }
 
@@ -145,6 +146,72 @@ enum SubresourceValue {
   SUBRESOURCE_VALUE_MAX
 };
 
+void Predictor::AnticipateOmniboxUrl(const GURL& url, bool preconnectable) {
+  std::string host = url.HostNoBrackets();
+  bool is_new_host_request = (host != last_omnibox_host_);
+  last_omnibox_host_ = host;
+
+  UrlInfo::ResolutionMotivation motivation(UrlInfo::OMNIBOX_MOTIVATED);
+  base::TimeTicks now = base::TimeTicks::Now();
+
+  if (preconnect_enabled()) {
+    if (preconnectable && !is_new_host_request) {
+      ++consecutive_omnibox_preconnect_count_;
+      // The omnibox suggests a search URL (for which we can preconnect) after
+      // one or two characters are typed, even though such typing often (1 in
+      // 3?) becomes a real URL.  This code waits till is has more evidence of a
+      // preconnectable URL (search URL) before forming a preconnection, so as
+      // to reduce the useless preconnect rate.
+      // Perchance this logic should be pushed back into the omnibox, where the
+      // actual characters typed, such as a space, can better forcast whether
+      // we need to search/preconnect or not.  By waiting for at least 4
+      // characters in a row that have lead to a search proposal, we avoid
+      // preconnections for a prefix like "www." and we also wait until we have
+      // at least a 4 letter word to search for.
+      // Each character typed appears to induce 2 calls to
+      // AnticipateOmniboxUrl(), so we double 4 characters and limit at 8
+      // requests.
+      // TODO(jar): Use an A/B test to optimize this.
+      const int kMinConsecutiveRequests = 8;
+      if (consecutive_omnibox_preconnect_count_ >= kMinConsecutiveRequests) {
+        // TODO(jar): The wild guess of 30 seconds could be tuned/tested, but it
+        // currently is just a guess that most sockets will remain open for at
+        // least 30 seconds.  This avoids a lot of cross-thread posting, and
+        // exercise of the network stack in this common case.
+        const int kMaxSearchKeepaliveSeconds(30);
+        if ((now - last_omnibox_preconnect_).InSeconds() <
+             kMaxSearchKeepaliveSeconds)
+          return;  // We've done a preconnect recently.
+        last_omnibox_preconnect_ = now;
+
+        Preconnect::PreconnectOnUIThread(CanonicalizeUrl(url), motivation);
+        return;  // Skip pre-resolution, since we'll open a connection.
+      }
+    } else {
+      consecutive_omnibox_preconnect_count_ = 0;
+    }
+  }
+
+  // Fall through and consider pre-resolution.
+
+  // Omnibox tends to call in pairs (just a few milliseconds apart), and we
+  // really don't need to keep resolving a name that often.
+  // TODO(jar): A/B tests could check for perf impact of the early returns.
+  if (!is_new_host_request) {
+    const int kMinPreresolveSeconds(10);
+    if (kMinPreresolveSeconds > (now - last_omnibox_preresolve_).InSeconds())
+      return;
+  }
+  last_omnibox_preresolve_ = now;
+
+  // Perform at least DNS pre-resolution.
+  ChromeThread::PostTask(
+      ChromeThread::IO,
+      FROM_HERE,
+      NewRunnableMethod(this, &Predictor::Resolve, CanonicalizeUrl(url),
+                        motivation));
+}
+
 void Predictor::PredictFrameSubresources(const GURL& url) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DCHECK(url.GetWithEmptyPath() == url);
@@ -588,4 +655,34 @@ void Predictor::DeserializeReferrersThenDelete(ListValue* referral_list) {
     delete referral_list;
 }
 
+
+//------------------------------------------------------------------------------
+// Helper functions
+//------------------------------------------------------------------------------
+
+// static
+GURL Predictor::CanonicalizeUrl(const GURL& url) {
+  if (!url.has_host())
+     return GURL::EmptyGURL();
+
+  std::string scheme;
+  if (url.has_scheme()) {
+    scheme = url.scheme();
+    if (scheme != "http" && scheme != "https")
+      return GURL::EmptyGURL();
+    if (url.has_port())
+      return url.GetWithEmptyPath();
+  } else {
+    scheme = "http";
+  }
+
+  // If we omit a port, it will default to 80 or 443 as appropriate.
+  std::string colon_plus_port;
+  if (url.has_port())
+    colon_plus_port = ":" + url.port();
+
+  return GURL(scheme + "://" + url.host() + colon_plus_port);
+}
+
+
 }  // namespace chrome_browser_net
diff --git a/chrome/browser/net/predictor.h b/chrome/browser/net/predictor.h
index 5d754ae..20fbe93 100644
--- a/chrome/browser/net/predictor.h
+++ b/chrome/browser/net/predictor.h
@@ -91,6 +91,11 @@ class Predictor : public base::RefCountedThreadSafe<Predictor> {
   // instigated this activity.
   void PredictFrameSubresources(const GURL& url);
 
+  // The Omnibox has proposed a given url to the user, and if it is a search
+  // URL, then it also indicates that this is preconnectable (i.e., we could
+  // preconnect to the search server).
+  void AnticipateOmniboxUrl(const GURL& url, bool preconnectable);
+
   // Record details of a navigation so that we can preresolve the host name
   // ahead of time the next time the users navigates to the indicated host.
   void LearnFromNavigation(const GURL& referring_url, const GURL& target_url);
@@ -128,6 +133,11 @@ class Predictor : public base::RefCountedThreadSafe<Predictor> {
   // Flag setting to use preconnection instead of just DNS pre-fetching.
   bool preconnect_enabled() const { return preconnect_enabled_; }
 
+  // Put URL in canonical form, including a scheme, host, and port.
+  // Returns GURL::EmptyGURL() if the scheme is not http/https or if the url
+  // cannot be otherwise canonicalized.
+  static GURL CanonicalizeUrl(const GURL& url);
+
  private:
   friend class base::RefCountedThreadSafe<Predictor>;
   FRIEND_TEST_ALL_PREFIXES(PredictorTest, BenefitLookupTest);
@@ -262,6 +272,19 @@ class Predictor : public base::RefCountedThreadSafe<Predictor> {
   // subresources and omni-box search URLs.
   bool preconnect_enabled_;
 
+  // Most recent suggestion from Omnibox provided via AnticipateOmniboxUrl().
+  std::string last_omnibox_host_;
+
+  // The time when the last preresolve was done for last_omnibox_host_.
+  base::TimeTicks last_omnibox_preresolve_;
+
+  // The number of consecutive requests to AnticipateOmniboxUrl() that suggested
+  // preconnecting (because it was to a search service).
+  int consecutive_omnibox_preconnect_count_;
+
+  // The time when the last preconnection was requested to a search service.
+  base::TimeTicks last_omnibox_preconnect_;
+
   DISALLOW_COPY_AND_ASSIGN(Predictor);
 };
 
diff --git a/chrome/browser/net/predictor_api.cc b/chrome/browser/net/predictor_api.cc
index c55fbf4..03b2004 100644
--- a/chrome/browser/net/predictor_api.cc
+++ b/chrome/browser/net/predictor_api.cc
@@ -35,9 +35,6 @@ using base::TimeDelta;
 
 namespace chrome_browser_net {
 
-static void ResolveOnUIThread(const GURL& url,
-                              UrlInfo::ResolutionMotivation motivation);
-
 static void DnsPrefetchMotivatedList(const UrlList& urls,
                                      UrlInfo::ResolutionMotivation motivation);
 
@@ -86,36 +83,6 @@ class InitialObserver {
 static InitialObserver* g_initial_observer = NULL;
 
 //------------------------------------------------------------------------------
-// Static helper functions
-//------------------------------------------------------------------------------
-
-// Put URL in canonical form, including a scheme, host, and port.
-// Returns GURL::EmptyGURL() if the scheme is not http/https or if the url
-// cannot be otherwise canonicalized.
-static GURL CanonicalizeUrl(const GURL& url) {
-  if (!url.has_host())
-     return GURL::EmptyGURL();
-
-  std::string scheme;
-  if (url.has_scheme()) {
-    scheme = url.scheme();
-    if (scheme != "http" && scheme != "https")
-      return GURL::EmptyGURL();
-    if (url.has_port())
-      return url.GetWithEmptyPath();
-  } else {
-    scheme = "http";
-  }
-
-  // If we omit a port, it will default to 80 or 443 as appropriate.
-  std::string colon_plus_port;
-  if (url.has_port())
-    colon_plus_port = ":" + url.port();
-
-  return GURL(scheme + "://" + url.host() + colon_plus_port);
-}
-
-//------------------------------------------------------------------------------
 // This section contains all the globally accessable API entry points for the
 // DNS Prefetching feature.
 //------------------------------------------------------------------------------
@@ -154,7 +121,7 @@ void RegisterUserPrefs(PrefService* user_prefs) {
 // When enabled, we use the following instance to service all requests in the
 // browser process.
 // TODO(willchan): Look at killing this.
-static Predictor* predictor = NULL;
+static Predictor* g_predictor = NULL;
 
 // This API is only used in the browser process.
 // It is called from an IPC message originating in the renderer.  It currently
@@ -179,16 +146,16 @@ static void DnsPrefetchMotivatedList(
     UrlInfo::ResolutionMotivation motivation) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI) ||
          ChromeThread::CurrentlyOn(ChromeThread::IO));
-  if (!predictor_enabled || NULL == predictor)
+  if (!predictor_enabled || NULL == g_predictor)
     return;
 
   if (ChromeThread::CurrentlyOn(ChromeThread::IO)) {
-    predictor->ResolveList(urls, motivation);
+    g_predictor->ResolveList(urls, motivation);
   } else {
     ChromeThread::PostTask(
         ChromeThread::IO,
         FROM_HERE,
-        NewRunnableMethod(predictor,
+        NewRunnableMethod(g_predictor,
                           &Predictor::ResolveList, urls, motivation));
   }
 }
@@ -196,60 +163,12 @@ static void DnsPrefetchMotivatedList(
 // This API is used by the autocomplete popup box (where URLs are typed).
 void AnticipateUrl(const GURL& url, bool preconnectable) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  if (!predictor_enabled || NULL == predictor)
+  if (!predictor_enabled || NULL == g_predictor)
     return;
-  if (!url.is_valid())
+  if (!url.is_valid() || !url.has_host())
     return;
 
-  static std::string last_host;
-  std::string host = url.HostNoBrackets();
-  bool is_new_host_request = (host != last_host);
-  last_host = host;
-
-  // Omnibox tends to call in pairs (just a few milliseconds apart), and we
-  // really don't need to keep resolving a name that often.
-  // TODO(jar): A/B tests could check for perf impact of the early returns.
-  static base::TimeTicks last_prefetch_for_host;
-  base::TimeTicks now = base::TimeTicks::Now();
-  if (!is_new_host_request) {
-    const int kMinPreresolveSeconds(10);
-    if (kMinPreresolveSeconds > (now - last_prefetch_for_host).InSeconds())
-      return;
-  }
-  last_prefetch_for_host = now;
-
-  GURL canonical_url(CanonicalizeUrl(url));
-  UrlInfo::ResolutionMotivation motivation(UrlInfo::OMNIBOX_MOTIVATED);
-
-  if (predictor->preconnect_enabled() && preconnectable) {
-    static base::TimeTicks last_keepalive;
-    // TODO(jar): The wild guess of 30 seconds could be tuned/tested, but it
-    // currently is just a guess that most sockets will remain open for at least
-    // 30 seconds.
-    const int kMaxSearchKeepaliveSeconds(30);
-    if ((now - last_keepalive).InSeconds() < kMaxSearchKeepaliveSeconds)
-      return;
-    last_keepalive = now;
-
-    Preconnect::PreconnectOnUIThread(canonical_url, motivation);
-    return;  // Skip pre-resolution, since we'll open a connection.
-  }
-
-  // Perform at least DNS pre-resolution.
-  ResolveOnUIThread(canonical_url, motivation);
-}
-
-static void ResolveOnUIThread(const GURL& url,
-                              UrlInfo::ResolutionMotivation motivation) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  if (!predictor_enabled || NULL == predictor || !url.has_host())
-    return;
-
-  ChromeThread::PostTask(
-      ChromeThread::IO,
-      FROM_HERE,
-      NewRunnableMethod(predictor,
-                        &Predictor::Resolve, url, motivation));
+  g_predictor->AnticipateOmniboxUrl(url, preconnectable);
 }
 
 //------------------------------------------------------------------------------
@@ -261,9 +180,9 @@ static void ResolveOnUIThread(const GURL& url,
 
 void PredictFrameSubresources(const GURL& url) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-  if (!predictor_enabled || NULL == predictor)
+  if (!predictor_enabled || NULL == g_predictor)
     return;
-  predictor->PredictFrameSubresources(url);
+  g_predictor->PredictFrameSubresources(url);
 }
 
 void LearnAboutInitialNavigation(const GURL& url) {
@@ -275,9 +194,9 @@ void LearnAboutInitialNavigation(const GURL& url) {
 
 void LearnFromNavigation(const GURL& referring_url, const GURL& target_url) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-  if (!predictor_enabled || NULL == predictor)
+  if (!predictor_enabled || NULL == g_predictor)
     return;
-  predictor->LearnFromNavigation(referring_url, target_url);
+  g_predictor->LearnFromNavigation(referring_url, target_url);
 }
 
 // The observer class needs to connect starts and finishes of HTTP network
@@ -290,7 +209,7 @@ typedef std::map<int, UrlInfo> ObservedResolutionMap;
 void InitialObserver::Append(const GURL& url) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  if (!on_the_record_switch || NULL == predictor)
+  if (!on_the_record_switch || NULL == g_predictor)
     return;
   if (kStartupResolutionCount <= first_navigations_.size())
     return;
@@ -311,7 +230,7 @@ void InitialObserver::GetInitialDnsResolutionList(ListValue* startup_list) {
   for (FirstNavigations::iterator it = first_navigations_.begin();
        it != first_navigations_.end();
        ++it) {
-    DCHECK(it->first == CanonicalizeUrl(it->first));
+    DCHECK(it->first == Predictor::CanonicalizeUrl(it->first));
     startup_list->Append(new StringValue(it->first.spec()));
   }
 }
@@ -399,7 +318,7 @@ void PredictorGetHtmlInfo(std::string* output) {
                  // We'd like the following no-cache... but it doesn't work.
                  // "<META HTTP-EQUIV=\"Pragma\" CONTENT=\"no-cache\">"
                  "</head><body>");
-  if (!predictor_enabled  || NULL == predictor) {
+  if (!predictor_enabled  || NULL == g_predictor) {
     output->append("Dns Prefetching is disabled.");
   } else {
     if (!on_the_record_switch) {
@@ -409,9 +328,9 @@ void PredictorGetHtmlInfo(std::string* output) {
       if (g_initial_observer)
         g_initial_observer->GetFirstResolutionsHtml(output);
       // Show list of subresource predictions and stats.
-      predictor->GetHtmlReferrerLists(output);
+      g_predictor->GetHtmlReferrerLists(output);
       // Show list of prediction results.
-      predictor->GetHtmlInfo(output);
+      g_predictor->GetHtmlInfo(output);
     }
   }
   output->append("</body></html>");
@@ -447,7 +366,7 @@ void FinalizePredictorInitialization(
     const UrlList& startup_urls,
     ListValue* referral_list) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-  predictor = global_predictor;
+  g_predictor = global_predictor;
   g_initial_observer = new InitialObserver();
 
   DLOG(INFO) << "DNS Prefetch service started";
@@ -455,12 +374,12 @@ void FinalizePredictorInitialization(
   // Prefetch these hostnames on startup.
   DnsPrefetchMotivatedList(startup_urls,
                            UrlInfo::STARTUP_LIST_MOTIVATED);
-  predictor->DeserializeReferrersThenDelete(referral_list);
+  g_predictor->DeserializeReferrersThenDelete(referral_list);
 }
 
 void FreePredictorResources() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
-  predictor = NULL;  // Owned and released by io_thread.cc.
+  g_predictor = NULL;  // Owned and released by io_thread.cc.
   delete g_initial_observer;
   g_initial_observer = NULL;
 }
@@ -475,7 +394,7 @@ static void SaveDnsPrefetchStateForNextStartupAndTrimOnIOThread(
     base::WaitableEvent* completion) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  if (NULL == predictor) {
+  if (NULL == g_predictor) {
     completion->Signal();
     return;
   }
@@ -487,8 +406,8 @@ static void SaveDnsPrefetchStateForNextStartupAndTrimOnIOThread(
   // of physical time, or perhaps after 48 hours of running (excluding time
   // between sessions possibly).
   // For now, we'll just trim at shutdown.
-  predictor->TrimReferrers();
-  predictor->SerializeReferrers(referral_list);
+  g_predictor->TrimReferrers();
+  g_predictor->SerializeReferrers(referral_list);
 
   completion->Signal();
 }
@@ -496,7 +415,7 @@ static void SaveDnsPrefetchStateForNextStartupAndTrimOnIOThread(
 void SavePredictorStateForNextStartupAndTrim(PrefService* prefs) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
-  if (!predictor_enabled || predictor == NULL)
+  if (!predictor_enabled || g_predictor == NULL)
     return;
 
   base::WaitableEvent completion(true, false);
@@ -570,7 +489,8 @@ static UrlList GetPredictedUrlListAtStartup(PrefService* user_prefs,
 
 //------------------------------------------------------------------------------
 // Methods for the helper class that is used to startup and teardown the whole
-// predictor system (both DNS pre-resolution and TCP/IP connection prewarming).
+// g_predictor system (both DNS pre-resolution and TCP/IP connection
+// prewarming).
 
 PredictorInit::PredictorInit(PrefService* user_prefs,
                              PrefService* local_state,
@@ -654,7 +574,7 @@ PredictorInit::PredictorInit(PrefService* user_prefs,
 
     Preconnect::SetPreconnectDespiteProxy(proconnect_despite_proxy);
 
-    DCHECK(!predictor);
+    DCHECK(!g_predictor);
     InitNetworkPredictor(max_queueing_delay, max_concurrent, user_prefs,
                          local_state, preconnect_enabled);
   }
diff --git a/chrome/browser/net/predictor_unittest.cc b/chrome/browser/net/predictor_unittest.cc
index 5bc650a..fc6ebfe 100644
--- a/chrome/browser/net/predictor_unittest.cc
+++ b/chrome/browser/net/predictor_unittest.cc
@@ -236,7 +236,8 @@ TEST_F(PredictorTest, MassiveConcurrentLookupTest) {
 
   UrlList names;
   for (int i = 0; i < 100; i++)
-    names.push_back(GURL("http://host" + base::IntToString(i) + ".notfound:80"));
+    names.push_back(GURL(
+        "http://host" + base::IntToString(i) + ".notfound:80"));
 
   // Try to flood the predictor with many concurrent requests.
   for (int i = 0; i < 10; i++)
@@ -540,4 +541,42 @@ TEST_F(PredictorTest, PriorityQueueReorderTest) {
   EXPECT_TRUE(queue.IsEmpty());
 }
 
+TEST_F(PredictorTest, CanonicalizeUrl) {
+  // Base case, only handles HTTP and HTTPS.
+  EXPECT_EQ(GURL(), Predictor::CanonicalizeUrl(GURL("ftp://anything")));
+
+  // Remove path testing.
+  GURL long_url("http://host:999/path?query=value");
+  EXPECT_EQ(Predictor::CanonicalizeUrl(long_url), long_url.GetWithEmptyPath());
+
+  // Default port cannoncalization.
+  GURL implied_port("http://test");
+  GURL explicit_port("http://test:80");
+  EXPECT_EQ(Predictor::CanonicalizeUrl(implied_port),
+            Predictor::CanonicalizeUrl(explicit_port));
+
+  // Port is still maintained.
+  GURL port_80("http://test:80");
+  GURL port_90("http://test:90");
+  EXPECT_NE(Predictor::CanonicalizeUrl(port_80),
+            Predictor::CanonicalizeUrl(port_90));
+
+  // Host is still maintained.
+  GURL host_1("http://test_1");
+  GURL host_2("http://test_2");
+  EXPECT_NE(Predictor::CanonicalizeUrl(host_1),
+            Predictor::CanonicalizeUrl(host_2));
+
+  // Scheme is maintained (mismatch identified).
+  GURL http("http://test");
+  GURL https("https://test");
+  EXPECT_NE(Predictor::CanonicalizeUrl(http),
+            Predictor::CanonicalizeUrl(https));
+
+  // Https works fine.
+  GURL long_https("https://host:999/path?query=value");
+  EXPECT_EQ(Predictor::CanonicalizeUrl(long_https),
+            long_https.GetWithEmptyPath());
+}
+
 }  // namespace chrome_browser_net