predictors: Use redirect data in prefetch.

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index 03e9570a0b1cc020038315f01d97dc01a6468e85..554b3e9781510bd78af157aab50b6d56bc3dfac9 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -210,6 +210,47 @@ content::ResourceType ResourcePrefetchPredictor::GetResourceTypeFromMimeType(
   return fallback;
 }
 
+// static
+bool ResourcePrefetchPredictor::GetRedirectEndpoint(
+    const std::string& first_redirect,
+    const RedirectDataMap& redirect_data_map,
+    std::string* final_redirect) {
+  DCHECK(final_redirect);
+
+  RedirectDataMap::const_iterator it = redirect_data_map.find(first_redirect);
+  if (it == redirect_data_map.end())
+    return false;
+
+  const RedirectData& redirect_data = it->second;
+  int best_redirect_i = -1;
+  float best_confidence = 0.0;
+  for (int i = 0; i < redirect_data.redirect_endpoints_size(); ++i) {
+    const RedirectStat& redirect = redirect_data.redirect_endpoints(i);
+    float confidence =
+        (redirect.number_of_hits() + 0.0) /
+        (redirect.number_of_hits() + redirect.number_of_misses());
+    if (confidence > best_confidence) {
+      best_redirect_i = i;
+      best_confidence = confidence;
+    }
+  }
+
+  if (best_redirect_i == -1)
+    return false;
+
+  const RedirectStat& best_redirect =
+      redirect_data.redirect_endpoints(best_redirect_i);
+  const float kMinRedirectConfidenceToTriggerPrefetch = 0.7f;
+  const int kMinRedirectHitsToTriggerPrefetch = 2;
+
+  if (best_confidence < kMinRedirectConfidenceToTriggerPrefetch ||
+      best_redirect.number_of_hits() < kMinRedirectHitsToTriggerPrefetch)
+    return false;
+
+  *final_redirect = best_redirect.url();
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ResourcePrefetchPredictor nested types.
 
@@ -419,9 +460,6 @@ void ResourcePrefetchPredictor::OnMainFrameRedirect(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // Stop any inflight prefetching. Remove the older navigation.
-  StopPrefetching(response.navigation_id.main_frame_url);
-
   std::unique_ptr<PageRequestSummary> summary;
   NavigationMap::iterator nav_it =
       inflight_navigations_.find(response.navigation_id);
@@ -492,47 +530,65 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
 bool ResourcePrefetchPredictor::GetPrefetchData(const GURL& main_frame_url,
                                                 std::vector<GURL>* urls) {
   DCHECK(urls);
-
+  DCHECK(urls->empty());
   bool use_url_data = config_.IsPrefetchingEnabled(profile_) ?
       config_.IsURLPrefetchingEnabled(profile_) :
       config_.IsURLLearningEnabled();
-  if (use_url_data) {
-    PrefetchDataMap::const_iterator iterator =
-        url_table_cache_->find(main_frame_url.spec());
-    if (iterator != url_table_cache_->end())
-      PopulatePrefetcherRequest(iterator->second, urls);
-  }
-  if (!urls->empty())
-    return true;
-
   bool use_host_data = config_.IsPrefetchingEnabled(profile_) ?
       config_.IsHostPrefetchingEnabled(profile_) :
       config_.IsHostLearningEnabled();
-  if (use_host_data) {
-    PrefetchDataMap::const_iterator iterator =
-        host_table_cache_->find(main_frame_url.host());
-    if (iterator != host_table_cache_->end())
-      PopulatePrefetcherRequest(iterator->second, urls);
+
+  if (use_url_data &&

[pasko, 2016/10/07 15:42:44]

add a comment above:
// Attempt to fetch URLs based on the incoming host/URL.

[alexilin, 2016/10/07 16:33:44]

Done.

+      PopulatePrefetcherRequest(main_frame_url.spec(), *url_table_cache_, urls))
+    return true;
+
+  if (use_host_data && PopulatePrefetcherRequest(main_frame_url.host(),
+                                                 *host_table_cache_, urls))
+    return true;
+
+  std::string redirect_endpoint;

[pasko, 2016/10/07 15:42:44]

Add a comment 1 line above saying:
// Fallback to fetching URLs based on a redirect endpoint for hist/URL.

[alexilin, 2016/10/07 16:33:44]

Done.

+  if (GetRedirectEndpoint(main_frame_url.spec(), *url_redirect_table_cache_,
+                          &redirect_endpoint)) {
+    GURL redirect_endpoint_url(redirect_endpoint);
+
+    if (use_url_data && PopulatePrefetcherRequest(redirect_endpoint_url.spec(),
+                                                  *url_table_cache_, urls))

[pasko, 2016/10/07 15:42:44]

if the condition takes multiple lines, it requires braces (used to be chromium
style, but I cannot find a reference easily - good for consistency)

[alexilin, 2016/10/07 16:33:44]

Oh, I've thought that this rule is applied only for single multiline statement
under condition. Good to know, thanks!

Quote from
https://google.github.io/styleguide/cppguide.html#Conditionals
"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces. Some projects
require that an if must always always have an accompanying brace."

So actually this rule isn't obligatory.

[pasko, 2016/10/07 17:18:54]

I was curious ... because I actually prefer always-braces.

The chromium style guide used to have this line:
"Omit {} for one-line conditional or loop bodies", but it was removed 4 years
ago without notice .. (I just traced it, rev#126 of [1] - pkasting@), somewhat
later there is a conversation on chromium-dev@ [2] where he expresses his
opinion:

"""
there are files and submodules within Chrome that require braces on all
conditionals; if you're modifying code in those, follow the prevailing practice.
 Most code does not use braces on one-line conditionals, and I am not a huge fan
of either changing it all or just instating a new rule, because the amount of
code affected is so large -- probably larger than with any other rule change
we've ever discussed.
"""

so indeed putting or not-putting braces is not obligatory, hence, as usual we
should aim at consistency with the current file/module.

In any case, this is a bit orthogonal, multi-line statements require braces :)

[1] https://sites.google.com/a/chromium.org/dev/developers/coding-style

[2]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bf385RfI_JQ/bON68...

[alexilin, 2016/10/11 13:14:41]

Benoit has already forced me to remove brackets in one-line if statement. So I'm
between two fires with you guys.
Done.

[pasko, 2016/10/11 14:16:19]

Sorry for the fires :)

I am with Benoit on one-line conditionals (for consistency), but this statement
did not fit one line.

[alexilin, 2016/10/11 15:24:29]

Yeah, I see that this is another case.
To summarize, no braces if the condition AND the statement fits in one line.
Otherwise, put braces.
It also depends on surrounding code, of course.

[pasko, 2016/10/11 15:57:55]

not only that, there are more stupid rules regarding the 'else' :)

in short: if each of the parts of the conditional statement take one line - no
braces, otherwise braces for everything.

Examples of good:
if (foo)
  DoFoo()
else if (bar)
  DoBar()

if (foo) {
  DoFoo()
} else if (Condition(multi,
                     line)) {
  DoBar()
}

+      return true;
+
+    if (use_host_data && PopulatePrefetcherRequest(redirect_endpoint_url.host(),

[pasko, 2016/10/07 15:42:45]

Using URL-based redirects for host-based prefetch has non-obvious benefits. Can
you explain the motivation? (we might have discussed it, and I just forgot)

At least it seems that host-based redirects should be tried first.

Because of this the code looks less symmetric and harder to read.

[alexilin, 2016/10/07 16:33:44]

It wasn't discussed yet so this is only my opinion.
Well, URL-based redirect seemingly should have more accurate stats than
host-based URL. Since we are confident in redirect endpoint, this redirect will
lead us to a web-page that user will likely see. Then why shouldn't we try
host-base prefetch in this case? Redirects and resources tables are not
connected actually. 

I'm not sure that we should try host-based redirects first for the same reason
that we don't try host-based resources first: it looks like URL data is more
"accurate". Moreover, if we have URL-based redirect for some URL then in most
cases we'll have host-based redirect for this URL. Hence there won't be any
sense to try URL-based redirects after host-based. 

[pasko, 2016/10/07 17:18:54]

Im, interesting and reasonable.

Theoretically .. it could be that the data for this particular URL does not have
many datapoints and hence has some randomness, and the host data is more
precise. In other words, I can see cases when your proposal is good and when it
is bad, but I don't have a sense of what is more likely. Perhaps we need a
whiteboard for that .. or a blackboard.

So, in presence of no clarity of the impact of this, I would prefer the code to
be simpler and more symmetric towards URL/host, it would be easier to
review/modify and perform data analysis if those two systems are more
independent.

[alexilin, 2016/10/11 13:14:41]

Done.

+                                                   *host_table_cache_, urls))
+      return true;
   }
 
-  return !urls->empty();
+  return use_host_data &&
+         GetRedirectEndpoint(main_frame_url.host(), *host_redirect_table_cache_,
+                             &redirect_endpoint) &&
+         PopulatePrefetcherRequest(redirect_endpoint, *host_table_cache_, urls);
 }
 
-void ResourcePrefetchPredictor::PopulatePrefetcherRequest(
-    const PrefetchData& data,
+bool ResourcePrefetchPredictor::PopulatePrefetcherRequest(
+    const std::string& main_frame_key,
+    const PrefetchDataMap& data_map,
     std::vector<GURL>* urls) {
-  for (const ResourceData& resource : data.resources()) {
+  DCHECK(urls);
+  PrefetchDataMap::const_iterator it = data_map.find(main_frame_key);
+  if (it == data_map.end())
+    return false;
+
+  size_t initial_size = urls->size();
+  for (const ResourceData& resource : it->second.resources()) {
     float confidence =
         static_cast<float>(resource.number_of_hits()) /
         (resource.number_of_hits() + resource.number_of_misses());
     if (confidence < config_.min_resource_confidence_to_trigger_prefetch ||
         resource.number_of_hits() <
-            config_.min_resource_hits_to_trigger_prefetch) {
+            config_.min_resource_hits_to_trigger_prefetch)
       continue;
-    }
 
     urls->push_back(GURL(resource.resource_url()));
   }
+
+  return urls->size() > initial_size;
 }
 
 void ResourcePrefetchPredictor::StartPrefetching(const GURL& url) {
@@ -1000,10 +1056,9 @@ void ResourcePrefetchPredictor::LearnRedirect(const std::string& key,
   }
 
   RedirectData& data = cache_entry->second;
-  // Trim and sort the redirects after the update.
+  // Trim the redirects after the update.
   ResourcePrefetchPredictorTables::TrimRedirects(
       &data, config_.max_consecutive_misses);
-  ResourcePrefetchPredictorTables::SortRedirects(&data);
 
   if (data.redirect_endpoints_size() == 0) {
     redirect_map->erase(cache_entry);