Stop initializing HintCache on UI thread

Previously, the HintCache's initialization, which involved adding all
hints contained within a hint vector to the cache, was being handled
on the UI thread. This initialization could take a significant amount
of time and triggered the creation of a jank startup performance bug.

The logic has been modified so that the HintCache is no longer
initialized on the UI thread. It is instead initialized within its
constructor during PreviewsHints::CreateFromHintsComponent() on a
background thread.

This is accomplished via a HintCache::Data object. Once the hints are
fully added to Data, it is moved into the HintCache's constructor. The
HintCache is immutable after construction.

Not only does this remove all of the costly UI thread processing, but
it also eliminates the expensive intermediate hint vector.

Bug: 910251
Change-Id: I04a28248ad1ce790f999da06f48e3c5aabfcb812
Reviewed-on: https://chromium-review.googlesource.com/c/1356226
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613196}

components/previews/content/hint_cache.cc

@@ -11,11 +11,27 @@ namespace previews {
 // Realistic minimum length of a host suffix.
 const int kMinHostSuffix = 6;  // eg., abc.tv
 
-HintCache::HintCache() {
-  DETACH_FROM_SEQUENCE(sequence_checker_);
+HintCache::Data::Data() = default;
+
+HintCache::Data::~Data() = default;
+
+void HintCache::Data::AddHint(const optimization_guide::proto::Hint& hint) {
+  DCHECK_EQ(optimization_guide::proto::HOST_SUFFIX, hint.key_representation());
+  activation_list_.insert(hint.key());
+  memory_cache_[hint.key()] = hint;
 }
 
-HintCache::~HintCache() {}
+bool HintCache::Data::HasHints() const {
+  return !activation_list_.empty();
+}
+
+HintCache::Data::Data(Data&& other) = default;
+
+HintCache::HintCache(Data&& data) : data_(std::move(data)) {
+  // Detach from sequence as the HintCache can be constructed on a different
+  // thread than it is used on.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
 bool HintCache::HasHint(const std::string& host) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -38,7 +54,7 @@ bool HintCache::IsHintLoaded(const std::string& host) const {
   if (host_suffix.empty()) {
     return false;
   }
-  return memory_cache_.find(host_suffix) != memory_cache_.end();
+  return data_.memory_cache_.find(host_suffix) != data_.memory_cache_.end();
 }
 
 const optimization_guide::proto::Hint* HintCache::GetHint(
@@ -48,31 +64,14 @@ const optimization_guide::proto::Hint* HintCache::GetHint(
   if (host_suffix.empty()) {
     return nullptr;
   }
-  auto it = memory_cache_.find(host_suffix);
-  if (it != memory_cache_.end()) {
+  auto it = data_.memory_cache_.find(host_suffix);
+  if (it != data_.memory_cache_.end()) {
     return &it->second;
   }
 
   return nullptr;
 }
 
-void HintCache::AddHint(const optimization_guide::proto::Hint& hint) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK_EQ(optimization_guide::proto::HOST_SUFFIX, hint.key_representation());
-
-  activation_list_.insert(hint.key());
-  // TODO(dougarnett): Limit size of memory cache.
-  memory_cache_[hint.key()] = hint;
-}
-
-void HintCache::AddHints(
-    const std::vector<optimization_guide::proto::Hint>& hints) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  for (auto hint : hints) {
-    AddHint(hint);
-  }
-}
-
 std::string HintCache::DetermineHostSuffix(const std::string& host) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   std::string suffix(host);
@@ -80,7 +79,7 @@ std::string HintCache::DetermineHostSuffix(const std::string& host) const {
   // lookups and substring work once get to a root domain like ".com" or
   // ".co.in" (MinHostSuffix length check is a heuristic for that).
   while (suffix.length() >= kMinHostSuffix) {
-    if (activation_list_.find(suffix) != activation_list_.end()) {
+    if (data_.activation_list_.find(suffix) != data_.activation_list_.end()) {
       return suffix;
     }
     size_t pos = suffix.find_first_of('.');

components/previews/content/hint_cache.h

@@ -8,7 +8,6 @@
 #include <map>
 #include <string>
 #include <unordered_set>
-#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -28,8 +27,44 @@ using HintLoadedCallback =
 // of the hints in a persisted backing store.
 class HintCache {
  public:
-  HintCache();
-  ~HintCache();
+  // Data is used to generate and store the hint data for the cache. After the
+  // hints from the hints protobuf are fully added to Data, it is moved into the
+  // HintCache constructor.
+  class Data {
+   public:
+    Data();
+    ~Data();
+
+    void AddHint(const optimization_guide::proto::Hint& hint);
+    bool HasHints() const;
+
+   private:
+    friend class HintCache;
+
+    // The move constructor is private, as it is only intended for use by the
+    // HintCache constructor. An rvalue reference to a HintCache::Data object is
+    // moved into the HintCache's |data_| member variable during construction.
+    Data(Data&& other);
+
+    // The set of host suffixes that have Hint data.
+    std::unordered_set<std::string> activation_list_;
+
+    // The in-memory cache of hints. Maps host suffix to Hint proto.
+    std::map<std::string, optimization_guide::proto::Hint> memory_cache_;
+
+    DISALLOW_COPY_AND_ASSIGN(Data);
+  };
+
+  // The hint cache can only be constructed from a Data object rvalue reference,
+  // which is used to move construct the HintCache's |data_| variable. After
+  // this, |data_| is immutable.
+  //
+  // Example:
+  //   HintCache::Data data;
+  //   data.AddHint(hint1);
+  //   data.AddHint(hint2);
+  //   HintCache hint_cache(std::move(data));
+  explicit HintCache(Data&& data);
 
   // Returns whether the cache has a hint data for |host| locally (whether
   // in memory or persisted on disk).
@@ -46,24 +81,16 @@ class HintCache {
   // Returns the hint data for |host| found in memory, otherwise nullptr.
   const optimization_guide::proto::Hint* GetHint(const std::string& host) const;
 
-  // Adds |hint| to the cache.
-  void AddHint(const optimization_guide::proto::Hint& hint);
-
-  // Adds |hints| to the cache.
-  void AddHints(const std::vector<optimization_guide::proto::Hint>& hints);
-
  private:
   // Returns the most specific host suffix of the host name that is present
   // in the activation list.
   std::string DetermineHostSuffix(const std::string& host) const;
 
-  // The set of host suffixes that have Hint data.
-  std::unordered_set<std::string> activation_list_;
-
-  // The in-memory cache of hints. Maps host suffix to Hint proto.
-  // TODO(dougarnett): Add MRU subset support (mru_cache) with backing store.
-  std::map<std::string, optimization_guide::proto::Hint> memory_cache_;
+  // |data_| contains all of the hint data available in the cache. It is
+  // immutable.
+  const Data data_;
 
+  // TODO(dougarnett): Add MRU subset support (mru_cache).
   // TODO(dougarnett): Add a backing store with all hints.
 
   SEQUENCE_CHECKER(sequence_checker_);

components/previews/content/hint_cache_unittest.cc

@@ -38,8 +38,6 @@ class HintCacheTest : public testing::Test {
 };
 
 TEST_F(HintCacheTest, TestMemoryCache) {
-  HintCache hint_cache;
-
   optimization_guide::proto::Hint hint1;
   hint1.set_key("subdomain.domain.org");
   hint1.set_key_representation(optimization_guide::proto::HOST_SUFFIX);
@@ -50,9 +48,11 @@ TEST_F(HintCacheTest, TestMemoryCache) {
   hint3.set_key("otherhost.subdomain.domain.org");
   hint3.set_key_representation(optimization_guide::proto::HOST_SUFFIX);
 
-  hint_cache.AddHint(hint1);
-  hint_cache.AddHint(hint2);
-  hint_cache.AddHint(hint3);
+  HintCache::Data data;
+  data.AddHint(hint1);
+  data.AddHint(hint2);
+  data.AddHint(hint3);
+  HintCache hint_cache(std::move(data));
 
   // Not matched
   EXPECT_FALSE(hint_cache.HasHint("domain.org"));
@@ -78,12 +78,13 @@ TEST_F(HintCacheTest, TestMemoryCache) {
 }
 
 TEST_F(HintCacheTest, TestMemoryCacheLoadCallback) {
-  HintCache hint_cache;
-
   optimization_guide::proto::Hint hint1;
   hint1.set_key("subdomain.domain.org");
   hint1.set_key_representation(optimization_guide::proto::HOST_SUFFIX);
-  hint_cache.AddHint(hint1);
+
+  HintCache::Data data;
+  data.AddHint(hint1);
+  HintCache hint_cache(std::move(data));
 
   EXPECT_TRUE(hint_cache.IsHintLoaded("host.subdomain.domain.org"));
   hint_cache.LoadHint(

components/previews/content/previews_hints.cc

@@ -180,13 +180,12 @@ bool IsDisabledExperimentalOptimization(
 // |total_page_patterns_with_resource_loading_hints_received| and
 // |total_resource_loading_hints_received| have their totals updated as
 // resource loading hints are encountered.
-void MaybeAddHintToStrippedHintsWithPageHints(
+void MaybeAddHintToStrippedHintsToHintCacheData(
     const optimization_guide::proto::Hint& hint,
-    std::vector<optimization_guide::proto::Hint>*
-        stripped_hints_with_page_hints,
+    HintCache::Data* hint_cache_data,
     size_t* total_page_patterns_with_resource_loading_hints_received,
     size_t* total_resource_loading_hints_received) {
-  DCHECK(stripped_hints_with_page_hints);
+  DCHECK(hint_cache_data);
   DCHECK(total_page_patterns_with_resource_loading_hints_received);
   DCHECK(total_resource_loading_hints_received);
 
@@ -271,7 +270,7 @@ void MaybeAddHintToStrippedHintsWithPageHints(
   if (stripped_hint.page_hints_size() > 0) {
     stripped_hint.set_key_representation(hint.key_representation());
     stripped_hint.set_key(hint.key());
-    stripped_hints_with_page_hints->push_back(stripped_hint);
+    hint_cache_data->AddHint(stripped_hint);
   }
 }
 
@@ -308,13 +307,13 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsComponent(
 
   base::FilePath sentinel_path(info.path.DirName().Append(kSentinelFileName));
   if (!CreateSentinelFile(sentinel_path, info.version)) {
-    std::unique_ptr<PreviewsHints> no_hints;
     RecordProcessHintsResult(
         PreviewsProcessHintsResult::kFailedFinishProcessing);
-    return no_hints;
+    return nullptr;
   }
 
   std::unique_ptr<PreviewsHints> hints(new PreviewsHints());
+  HintCache::Data hint_cache_data;
 
   // The condition set ID is a simple increasing counter that matches the
   // order of hints in the config (where earlier hints in the config take
@@ -331,15 +330,17 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsComponent(
   for (const auto& hint : config->hints()) {
     // We only support host suffixes at the moment. Skip anything else.
     // One |hint| applies to one host URL suffix.
-    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX)
+    if (hint.key_representation() != optimization_guide::proto::HOST_SUFFIX) {
       continue;
+    }
 
     const std::string& hint_key = hint.key();
 
     // Validate configuration keys.
     DCHECK(!hint_key.empty());
-    if (hint_key.empty())
+    if (hint_key.empty()) {
       continue;
+    }
 
     auto seen_host_suffixes_iter = seen_host_suffixes.find(hint_key);
     DCHECK(seen_host_suffixes_iter == seen_host_suffixes.end());
@@ -385,8 +386,8 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsComponent(
     // hint to the initial hints that are used to populate the hint cache,
     // removing all of the hint's top-level optimizations and only retaining the
     // first enabled optimization of each preview type within each page hint.
-    MaybeAddHintToStrippedHintsWithPageHints(
-        hint, &hints->initial_hints_,
+    MaybeAddHintToStrippedHintsToHintCacheData(
+        hint, &hint_cache_data,
         &total_page_patterns_with_resource_loading_hints_received,
         &total_resource_loading_hints_received);
   }
@@ -404,13 +405,18 @@ std::unique_ptr<PreviewsHints> PreviewsHints::CreateFromHintsComponent(
     hints->url_matcher_.AddConditionSets(all_conditions);
   }
 
+  if (hint_cache_data.HasHints()) {
+    hints->hint_cache_ =
+        std::make_unique<HintCache>(std::move(hint_cache_data));
+  }
+
   // Extract any supported large scale blacklists from the configuration.
   hints->ParseOptimizationFilters(*config);
 
   // Completed processing hints data without crashing so clear sentinel.
   DeleteSentinelFile(sentinel_path);
   RecordProcessHintsResult(
-      hints->initial_hints_.empty() && all_conditions.empty()
+      all_conditions.empty() && !hints->hint_cache_
           ? PreviewsProcessHintsResult::kProcessedNoPreviewsHints
           : PreviewsProcessHintsResult::kProcessedPreviewsHints);
   return hints;
@@ -450,7 +456,7 @@ void PreviewsHints::ParseOptimizationFilters(
             PreviewsOptimizationFilterStatus::kFailedServerBlacklistBadConfig);
         continue;
       }
-      if ((int)bloom_filter_proto.num_bits() >
+      if (static_cast<int>(bloom_filter_proto.num_bits()) >
           previews::params::
                   LitePageRedirectPreviewMaxServerBlacklistByteSize() /
               8) {
@@ -503,16 +509,6 @@ const optimization_guide::proto::PageHint* PreviewsHints::FindPageHint(
   return nullptr;
 }
 
-void PreviewsHints::Initialize() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!initial_hints_.empty()) {
-    if (!hint_cache_)
-      hint_cache_ = std::make_unique<HintCache>();
-    hint_cache_->AddHints(initial_hints_);
-    initial_hints_.clear();
-  }
-}
-
 bool PreviewsHints::IsWhitelisted(
     const GURL& url,
     PreviewsType type,
@@ -520,8 +516,9 @@ bool PreviewsHints::IsWhitelisted(
     net::EffectiveConnectionType* out_ect_threshold) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!url.has_host())
+  if (!url.has_host()) {
     return false;
+  }
 
   return IsWhitelistedAtTopLevel(url, type, out_inflation_percent) ||
          IsWhitelistedInPageHints(url, type, out_inflation_percent,
@@ -576,8 +573,9 @@ bool PreviewsHints::IsWhitelistedInPageHints(
     net::EffectiveConnectionType* out_ect_threshold) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!hint_cache_)
+  if (!hint_cache_) {
     return false;
+  }
 
   // Now check HintCache for a loaded entry with a PageHint that matches |url|
   // that whitelists the optimization.
@@ -616,8 +614,9 @@ bool PreviewsHints::IsWhitelistedInPageHints(
 bool PreviewsHints::IsBlacklisted(const GURL& url, PreviewsType type) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!url.has_host())
+  if (!url.has_host()) {
     return false;
+  }
 
   // Check large scale blacklists received from the server.
   // (At some point, we may have blacklisting to check in HintCache as well.)
@@ -639,8 +638,9 @@ bool PreviewsHints::MaybeLoadOptimizationHints(
     HintLoadedCallback callback) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!hint_cache_ || !url.has_host())
+  if (!hint_cache_ || !url.has_host()) {
     return false;
+  }
 
   hint_cache_->LoadHint(url.host(), std::move(callback));
   return hint_cache_->HasHint(url.host());
@@ -649,8 +649,9 @@ bool PreviewsHints::MaybeLoadOptimizationHints(
 void PreviewsHints::LogHintCacheMatch(const GURL& url,
                                       bool is_committed,
                                       net::EffectiveConnectionType ect) const {
-  if (!hint_cache_)
+  if (!hint_cache_) {
     return;
+  }
 
   if (hint_cache_->HasHint(url.host())) {
     if (!is_committed) {

components/previews/content/previews_hints.h

@@ -49,8 +49,6 @@ class PreviewsHints {
       const GURL& document_url,
       const optimization_guide::proto::Hint& hint);
 
-  void Initialize();
-
   // Whether the URL is whitelisted for the given previews type. If so,
   // |out_inflation_percent| and |out_ect_threshold| will be populated if
   // metadata is available for them. This first checks the top-level whitelist
@@ -116,8 +114,6 @@ class PreviewsHints {
            std::set<std::pair<PreviewsType, int>>>
       whitelist_;
 
-  std::vector<optimization_guide::proto::Hint> initial_hints_;
-
   // Blacklist of host suffixes for LITE_PAGE_REDIRECT Previews.
   std::unique_ptr<HostFilter> lite_page_redirect_blacklist_;
 

components/previews/content/previews_hints_unittest.cc

@@ -47,7 +47,6 @@ class PreviewsHintsTest : public testing::Test {
         temp_dir_.GetPath().Append(FILE_PATH_LITERAL("somefile.pb")));
     ASSERT_NO_FATAL_FAILURE(WriteConfigToFile(config, info.path));
     previews_hints_ = PreviewsHints::CreateFromHintsComponent(info);
-    previews_hints_->Initialize();
   }
 
   PreviewsHints* previews_hints() { return previews_hints_.get(); }

components/previews/content/previews_optimization_guide.cc

@@ -140,9 +140,6 @@ void PreviewsOptimizationGuide::UpdateHints(
     std::unique_ptr<PreviewsHints> hints) {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   hints_ = std::move(hints);
-  if (hints_) {
-    hints_->Initialize();
-  }
 
   // Record the result of updating the hints. This is used as a signal for the
   // hints being fully processed in testing.