predictors: Prefetch resources from manifest.

chrome/browser/predictors/resource_prefetch_predictor.cc

 // Copyright 2014 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 "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
@@ skipping 82 matching lines @@
 void InitializeOriginStatFromOriginRequestSummary(
     OriginStat* origin,
     const ResourcePrefetchPredictor::OriginRequestSummary& summary) {
   origin->set_origin(summary.origin.spec());
   origin->set_number_of_hits(1);
   origin->set_average_position(summary.first_occurrence + 1);
   origin->set_always_access_network(summary.always_access_network);
   origin->set_accessed_network(summary.accessed_network);
 }
 
+bool IsManifestTooOld(const precache::PrecacheManifest& manifest) {
+  const base::TimeDelta kMaxManifestAge = base::TimeDelta::FromDays(5);

[alexilin, 2017/04/18 07:05:03]

I don't feel strongly about this. 
Probably, it would be better to add the timestamp of the last fetch instead of
using manifest id.

+  return base::Time::Now() - base::Time::FromDoubleT(manifest.id().id()) >
+         kMaxManifestAge;
+}
+
 // Used to fetch the visit count for a URL from the History database.
 class GetUrlVisitCountTask : public history::HistoryDBTask {
  public:
   using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
   using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
   typedef base::Callback<void(size_t,  // URL visit count.
                               const PageRequestSummary&)>
       VisitInfoCallback;
 
   GetUrlVisitCountTask(std::unique_ptr<PageRequestSummary> summary,
@@ skipping 778 matching lines @@
       prediction->is_host = false;
       prediction->main_frame_key = redirect_endpoint;
       prediction->is_redirected = (redirect_endpoint != main_frame_url_spec);
     }
     return true;
   }
 
   // Use host data if the URL-based prediction isn't available.
   std::string main_frame_url_host = main_frame_url.host();
   if (GetRedirectEndpoint(main_frame_url_host, *host_redirect_table_cache_,
-                          &redirect_endpoint) &&
-      PopulatePrefetcherRequest(redirect_endpoint, *host_table_cache_, urls)) {
-    if (prediction) {
-      prediction->is_host = true;
-      prediction->main_frame_key = redirect_endpoint;
-      prediction->is_redirected = (redirect_endpoint != main_frame_url_host);
+                          &redirect_endpoint)) {
+    if (PopulatePrefetcherRequest(redirect_endpoint, *host_table_cache_,
+                                  urls)) {
+      if (prediction) {
+        prediction->is_host = true;
+        prediction->main_frame_key = redirect_endpoint;
+        prediction->is_redirected = (redirect_endpoint != main_frame_url_host);
+      }
+      return true;
     }
-    return true;
+
+    if (config_.is_manifests_enabled) {
+      // Use manifest data for host if available.
+      std::string manifest_host = redirect_endpoint;
+      if (base::StartsWith(manifest_host, "www.", base::CompareCase::SENSITIVE))

[Benoit L, 2017/04/18 16:28:39]

is www systematically stripped from the manifest keys?

[alexilin, 2017/04/19 07:59:25]

Yes. (See history::HostForTopHosts and how it's used for manifests)

+        manifest_host.assign(manifest_host, 4, std::string::npos);
+      if (PopulatePrefetcherRequestByManifest(manifest_host, urls)) {
+        if (prediction) {
+          prediction->is_host = true;
+          prediction->main_frame_key = redirect_endpoint;
+          prediction->is_redirected =
+              (redirect_endpoint != main_frame_url_host);
+        }
+        return true;
+      }
+    }
   }
-
   return false;
 }
 
 bool ResourcePrefetchPredictor::PopulatePrefetcherRequest(
     const std::string& main_frame_key,
     const PrefetchDataMap& data_map,
     std::vector<GURL>* urls) const {
   PrefetchDataMap::const_iterator it = data_map.find(main_frame_key);
   if (it == data_map.end())
     return false;
 
   bool has_prefetchable_resource = false;
   for (const ResourceData& resource : it->second.resources()) {
     if (IsResourcePrefetchable(resource)) {
       has_prefetchable_resource = true;
       if (urls)
         urls->push_back(GURL(resource.resource_url()));
     }
   }
 
   return has_prefetchable_resource;
 }
 
+bool ResourcePrefetchPredictor::PopulatePrefetcherRequestByManifest(
+    const std::string& manifest_host,
+    std::vector<GURL>* urls) const {
+  auto it = manifest_table_cache_->find(manifest_host);
+  if (it == manifest_table_cache_->end())
+    return false;
+
+  const precache::PrecacheManifest& manifest = it->second;
+
+  if (IsManifestTooOld(manifest))
+    return false;
+
+  const float kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;

[Benoit L, 2017/04/18 16:28:39]

Have you looked at the practical meaning of this threshold?


Also, tiny nit: since the name is really local, no need to make it that long :-)

[alexilin, 2017/04/19 07:59:25]

Roughly speaking, it's percentage of host pages where the resource is used. In
addition, pages are also weighted.
 
Done.

[Benoit L, 2017/04/19 14:48:54]

Ok, so can you add a comment saying that this is roughly in line with the
threshold we use in the rest of the code?

[alexilin, 2017/04/19 15:35:11]

Done.

+
+  base::Optional<std::vector<bool>> unused_bitset =
+      precache::GetResourceBitset(manifest, internal::kUnusedRemovedExperiment);
+  base::Optional<std::vector<bool>> versioned_bitset =
+      precache::GetResourceBitset(manifest,
+                                  internal::kVersionedRemovedExperiment);
+  base::Optional<std::vector<bool>> no_store_bitset =
+      precache::GetResourceBitset(manifest,
+                                  internal::kNoStoreRemovedExperiment);
+
+  bool has_prefetchable_resource = false;
+  for (int i = 0; i < manifest.resource_size(); ++i) {
+    const precache::PrecacheResource& resource = manifest.resource(i);
+    if (resource.weight_ratio() > kMinPrecacheResourceWeightToTriggerPrefetch &&
+        (!unused_bitset.has_value() || unused_bitset.value()[i]) &&

[Benoit L, 2017/04/18 16:28:39]

what is the meaning of the bits?

if unused, versioned and no_store are all set, is the resource prefetchable?


Update: after offline discussion, the logic is correct, but the naming weird.
Can you add a comment?

[alexilin, 2017/04/19 07:59:25]

Renamed and commented.
Done.

+        (!versioned_bitset.has_value() || versioned_bitset.value()[i]) &&
+        (!no_store_bitset.has_value() || no_store_bitset.value()[i])) {
+      has_prefetchable_resource = true;
+      if (urls)
+        urls->push_back(GURL(resource.url()));

[Benoit L, 2017/04/18 16:28:39]

useless_nit: emplace_back?

[alexilin, 2017/04/19 07:59:25]

Done.

+    }
+  }
+
+  return has_prefetchable_resource;
+}
+
 void ResourcePrefetchPredictor::CreateCaches(
     std::unique_ptr<PrefetchDataMap> url_data_map,
     std::unique_ptr<PrefetchDataMap> host_data_map,
     std::unique_ptr<RedirectDataMap> url_redirect_data_map,
     std::unique_ptr<RedirectDataMap> host_redirect_data_map,
     std::unique_ptr<ManifestDataMap> manifest_data_map,
     std::unique_ptr<OriginDataMap> origin_data_map) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   DCHECK_EQ(INITIALIZING, initialization_state_);
@@ skipping 686 matching lines @@
   }
 }
 
 void ResourcePrefetchPredictor::OnManifestFetched(
     const std::string& host,
     const precache::PrecacheManifest& manifest) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (initialization_state_ != INITIALIZED)
     return;
 
-  if (!config_.is_manifests_enabled)
+  if (!config_.is_manifests_enabled || IsManifestTooOld(manifest))
     return;
 
   // The manifest host has "www." prefix stripped, the manifest host could
   // correspond to two different hosts in the prefetch database.
   UpdatePrefetchDataByManifest(host, PREFETCH_KEY_TYPE_HOST,
                                host_table_cache_.get(), manifest);
   UpdatePrefetchDataByManifest("www." + host, PREFETCH_KEY_TYPE_HOST,
                                host_table_cache_.get(), manifest);
 
   // The manifest is too large to store.
@@ skipping 30 matching lines @@
 
   auto resource_entry = data_map->find(key);
   if (resource_entry == data_map->end())
     return;
 
   PrefetchData& data = resource_entry->second;
   std::map<std::string, int> manifest_index;
   for (int i = 0; i < manifest.resource_size(); ++i)
     manifest_index.insert({manifest.resource(i).url(), i});
 
-  bool was_updated = false;
   base::Optional<std::vector<bool>> unused_bitset =
       precache::GetResourceBitset(manifest, internal::kUnusedRemovedExperiment);
+
+  bool was_updated = false;
   if (unused_bitset.has_value()) {
     // Remove unused resources from |data|.
     auto new_end = std::remove_if(
         data.mutable_resources()->begin(), data.mutable_resources()->end(),
         [&](const ResourceData& x) {
           auto it = manifest_index.find(x.resource_url());
           if (it == manifest_index.end())
             return false;
           return !unused_bitset.value()[it->second];
         });
@@ skipping 30 matching lines @@
 TestObserver::~TestObserver() {
   predictor_->SetObserverForTesting(nullptr);
 }
 
 TestObserver::TestObserver(ResourcePrefetchPredictor* predictor)
     : predictor_(predictor) {
   predictor_->SetObserverForTesting(this);
 }
 
 }  // namespace predictors