Allow for experimenting with no persistent store for hints

Bug: 1112500
Change-Id: Icdf1aed836db7eb60189709dc1a9a02ca3142420
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335506
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794749}

chrome/browser/optimization_guide/hints_fetcher_browsertest.cc

@@ -751,18 +751,6 @@ IN_PROC_BROWSER_TEST_F(
       1);
   histogram_tester->ExpectTotalCount(
       "OptimizationGuide.HintsFetcher.GetHintsRequest.HintCount", 0);
-
-  LoadHintsForUrl(https_url());
-
-  ui_test_utils::NavigateToURL(browser(), https_url());
-
-  // Verifies that no Fetched Hint was added to the store, only the
-  // Component hint is loaded.
-  histogram_tester->ExpectUniqueSample(
-      "OptimizationGuide.HintCache.HintType.Loaded",
-      static_cast<int>(optimization_guide::OptimizationGuideStore::
-                           StoreEntryType::kComponentHint),
-      1);
 }
 
 IN_PROC_BROWSER_TEST_F(

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -238,11 +238,14 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
       profile_(profile),
       pref_service_(pref_service),
       hint_cache_(std::make_unique<optimization_guide::HintCache>(
-          std::make_unique<optimization_guide::OptimizationGuideStore>(
-              database_provider,
-              profile_path.AddExtensionASCII(
-                  optimization_guide::kOptimizationGuideHintStore),
-              background_task_runner_))),
+          optimization_guide::features::ShouldPersistHintsToDisk()
+              ? std::make_unique<optimization_guide::OptimizationGuideStore>(
+                    database_provider,
+                    profile_path.AddExtensionASCII(
+                        optimization_guide::kOptimizationGuideHintStore),
+                    background_task_runner_)
+              : nullptr,
+          optimization_guide::features::MaxHostKeyedHintCacheSize())),
       page_navigation_hints_fetchers_(
           optimization_guide::features::MaxConcurrentPageNavigationFetches()),
       hints_fetcher_factory_(
@@ -359,6 +362,11 @@ OptimizationGuideHintsManager::ProcessHintsComponent(
                              config->optimization_blacklists(),
                              registered_optimization_types);
 
+  // TODO(crbug/1112500): Figure out what to do with component hints if there
+  // isn't a persistent store. Right now, it doesn't really matter since there
+  // aren't hints sent down via the component, but we need to figure out
+  // threading since these hints are now stored in memory prior to being
+  // persisted.
   if (update_data) {
     bool did_process_hints = hint_cache_->ProcessAndCacheHints(
         config->mutable_hints(), update_data.get());
@@ -679,6 +687,14 @@ void OptimizationGuideHintsManager::OnFetchedTopHostsHintsStored() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   LOCAL_HISTOGRAM_BOOLEAN("OptimizationGuide.FetchedHints.Stored", true);
 
+  if (!optimization_guide::features::ShouldPersistHintsToDisk()) {
+    // If we aren't persisting hints to disk, there's no point in purging
+    // hints from disk or starting a new fetch since at this point we should
+    // just be fetching everything on page navigation and only storing
+    // in-memory.
+    return;
+  }
+
   hint_cache_->PurgeExpiredFetchedHints();
 
   top_hosts_hints_fetch_timer_.Stop();

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -266,7 +266,11 @@ class TestHintsFetcherFactory : public optimization_guide::HintsFetcherFactory {
 class OptimizationGuideHintsManagerTest
     : public optimization_guide::ProtoDatabaseProviderTestBase {
  public:
-  OptimizationGuideHintsManagerTest() = default;
+  OptimizationGuideHintsManagerTest() {
+    scoped_feature_list_.InitAndEnableFeatureWithParameters(
+        optimization_guide::features::kOptimizationHints,
+        {{"max_host_keyed_hint_cache_size", "1"}});
+  }
   ~OptimizationGuideHintsManagerTest() override = default;
 
   void SetUp() override {
@@ -364,6 +368,14 @@ class OptimizationGuideHintsManagerTest
     optimization_guide::proto::PreviewsMetadata* default_opt_metadata =
         default_opt->mutable_previews_metadata();
     default_opt_metadata->set_inflation_percent(1234);
+    // Add another hint so somedomain.org hint is not in-memory initially.
+    optimization_guide::proto::Hint* hint2 = config.add_hints();
+    hint2->set_key("somedomain2.org");
+    hint2->set_key_representation(optimization_guide::proto::HOST);
+    hint2->set_version("someversion");
+    optimization_guide::proto::Optimization* opt =
+        hint2->add_whitelisted_optimizations();
+    opt->set_optimization_type(optimization_guide::proto::NOSCRIPT);
 
     ProcessHints(config, version);
   }
@@ -438,6 +450,7 @@ class OptimizationGuideHintsManagerTest
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::MainThreadType::UI,
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::test::ScopedFeatureList scoped_feature_list_;
   TestingProfile testing_profile_;
   std::unique_ptr<content::TestWebContentsFactory> web_contents_factory_;
   std::unique_ptr<OptimizationGuideHintsManager> hints_manager_;

components/optimization_guide/hint_cache.cc

@@ -15,66 +15,63 @@
 
 namespace optimization_guide {
 
-namespace {
-
-// The default number of host-keyed hints retained within the host keyed cache.
-// When the limit is exceeded, the least recently used hint is purged from
-// |host_keyed_cache_|.
-const size_t kDefaultMaxMemoryCacheHostKeyedHints = 20;
-
-}  // namespace
-
 HintCache::HintCache(
     std::unique_ptr<OptimizationGuideStore> optimization_guide_store,
-    base::Optional<int>
-        max_memory_cache_host_keyed_hints /*= base::Optional<int>()*/)
+    int max_memory_cache_host_keyed_hints)
     : optimization_guide_store_(std::move(optimization_guide_store)),
-      host_keyed_cache_(std::max(max_memory_cache_host_keyed_hints.value_or(
-                                     kDefaultMaxMemoryCacheHostKeyedHints),
-                                 1)),
+      host_keyed_cache_(max_memory_cache_host_keyed_hints),
       url_keyed_hint_cache_(features::MaxURLKeyedHintCacheSize()),
-      clock_(base::DefaultClock::GetInstance()) {
-  DCHECK(optimization_guide_store_);
-}
+      clock_(base::DefaultClock::GetInstance()) {}
 
 HintCache::~HintCache() = default;
 
 void HintCache::Initialize(bool purge_existing_data,
                            base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  optimization_guide_store_->Initialize(
-      purge_existing_data,
-      base::BindOnce(&HintCache::OnStoreInitialized, base::Unretained(this),
-                     std::move(callback)));
+
+  if (optimization_guide_store_) {
+    optimization_guide_store_->Initialize(
+        purge_existing_data,
+        base::BindOnce(&HintCache::OnStoreInitialized, base::Unretained(this),
+                       std::move(callback)));
+    return;
+  }
+  std::move(callback).Run();
 }
 
 std::unique_ptr<StoreUpdateData>
 HintCache::MaybeCreateUpdateDataForComponentHints(
     const base::Version& version) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return optimization_guide_store_->MaybeCreateUpdateDataForComponentHints(
-      version);
+  if (optimization_guide_store_) {
+    return optimization_guide_store_->MaybeCreateUpdateDataForComponentHints(
+        version);
+  }
+  return nullptr;
 }
 
 std::unique_ptr<StoreUpdateData> HintCache::CreateUpdateDataForFetchedHints(
     base::Time update_time) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return optimization_guide_store_->CreateUpdateDataForFetchedHints(
-      update_time);
+  if (optimization_guide_store_) {
+    return optimization_guide_store_->CreateUpdateDataForFetchedHints(
+        update_time);
+  }
+  return nullptr;
 }
 
 void HintCache::UpdateComponentHints(
     std::unique_ptr<StoreUpdateData> component_data,
     base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(component_data);
-
-  // Clear the host-keyed cache prior to updating the store with the new
-  // component data.
-  host_keyed_cache_.Clear();
 
-  optimization_guide_store_->UpdateComponentHints(std::move(component_data),
-                                                  std::move(callback));
+  if (optimization_guide_store_) {
+    DCHECK(component_data);
+    optimization_guide_store_->UpdateComponentHints(std::move(component_data),
+                                                    std::move(callback));
+    return;
+  }
+  std::move(callback).Run();
 }
 
 void HintCache::UpdateFetchedHints(
@@ -93,29 +90,34 @@ void HintCache::UpdateFetchedHints(
 
   ProcessAndCacheHints(get_hints_response.get()->mutable_hints(),
                        fetched_hints_update_data.get());
-  optimization_guide_store_->UpdateFetchedHints(
-      std::move(fetched_hints_update_data), std::move(callback));
+
+  if (optimization_guide_store_) {
+    optimization_guide_store_->UpdateFetchedHints(
+        std::move(fetched_hints_update_data), std::move(callback));
+  } else {
+    std::move(callback).Run();
+  }
 }
 
 void HintCache::PurgeExpiredFetchedHints() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(optimization_guide_store_);
 
-  optimization_guide_store_->PurgeExpiredFetchedHints();
+  if (optimization_guide_store_)
+    optimization_guide_store_->PurgeExpiredFetchedHints();
 }
 
 void HintCache::ClearFetchedHints() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(optimization_guide_store_);
   url_keyed_hint_cache_.Clear();
   ClearHostKeyedHints();
 }
 
 void HintCache::ClearHostKeyedHints() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(optimization_guide_store_);
   host_keyed_cache_.Clear();
-  optimization_guide_store_->ClearFetchedHintsFromDatabase();
+
+  if (optimization_guide_store_)
+    optimization_guide_store_->ClearFetchedHintsFromDatabase();
 }
 
 bool HintCache::HasHint(const std::string& host) {
@@ -123,9 +125,12 @@ bool HintCache::HasHint(const std::string& host) {
 
   auto hint_it = host_keyed_cache_.Get(host);
   if (hint_it == host_keyed_cache_.end()) {
-    // If not in-memory, check database.
-    OptimizationGuideStore::EntryKey hint_entry_key;
-    return optimization_guide_store_->FindHintEntryKey(host, &hint_entry_key);
+    if (optimization_guide_store_) {
+      // If not in-memory, check database.
+      OptimizationGuideStore::EntryKey hint_entry_key;
+      return optimization_guide_store_->FindHintEntryKey(host, &hint_entry_key);
+    }
+    return false;
   }
 
   // The hint for |host| was requested but no hint was returned.
@@ -148,6 +153,11 @@ void HintCache::LoadHint(const std::string& host, HintLoadedCallback callback) {
   // there, then asynchronously load it from the store and return.
   auto hint_it = host_keyed_cache_.Get(host);
   if (hint_it == host_keyed_cache_.end()) {
+    if (!optimization_guide_store_) {
+      std::move(callback).Run(nullptr);
+      return;
+    }
+
     OptimizationGuideStore::EntryKey hint_entry_key;
     if (!optimization_guide_store_->FindHintEntryKey(host, &hint_entry_key)) {
       std::move(callback).Run(nullptr);
@@ -242,14 +252,14 @@ bool HintCache::HasURLKeyedEntryForURL(const GURL& url) {
 }
 
 base::Time HintCache::GetFetchedHintsUpdateTime() const {
-  if (!optimization_guide_store_) {
-    return base::Time();
-  }
-  return optimization_guide_store_->GetFetchedHintsUpdateTime();
+  if (optimization_guide_store_)
+    return optimization_guide_store_->GetFetchedHintsUpdateTime();
+  return base::Time();
 }
 
 void HintCache::OnStoreInitialized(base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(optimization_guide_store_);
   std::move(callback).Run();
 }
 
@@ -281,9 +291,11 @@ void HintCache::OnLoadStoreHint(
 bool HintCache::ProcessAndCacheHints(
     google::protobuf::RepeatedPtrField<proto::Hint>* hints,
     optimization_guide::StoreUpdateData* update_data) {
-  // If there's no update data, then there's nothing to do.
-  if (!update_data)
+  if (optimization_guide_store_ && !update_data) {
+    // If there's no update data, then there's nothing to do.
     return false;
+  }
+
   bool processed_hints_to_store = false;
   // Process each hint in the the hint configuration. The hints are mutable
   // because once processing is completed on each individual hint, it is moved
@@ -313,7 +325,8 @@ bool HintCache::ProcessAndCacheHints(
             std::make_unique<MemoryHint>(
                 expiry_time,
                 std::make_unique<optimization_guide::proto::Hint>(hint)));
-        update_data->MoveHintIntoUpdateData(std::move(hint));
+        if (update_data)
+          update_data->MoveHintIntoUpdateData(std::move(hint));
         processed_hints_to_store = true;
         break;
       case proto::FULL_URL:
@@ -347,8 +360,11 @@ void HintCache::AddHintForTesting(const GURL& url,
 
 bool HintCache::IsHintStoreAvailable() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(optimization_guide_store_);
-  return optimization_guide_store_->IsAvailable();
+
+  if (optimization_guide_store_)
+    return optimization_guide_store_->IsAvailable();
+
+  return false;
 }
 
 }  // namespace optimization_guide

components/optimization_guide/hint_cache.h

@@ -33,18 +33,17 @@ using HintLoadedCallback = base::OnceCallback<void(const proto::Hint*)>;
 // synchronously retrieve recently loaded hints keyed by URL or host.
 class HintCache {
  public:
-  // Construct the HintCache with a backing store and an optional max host-keyed
-  // cache size. While |optimization_guide_store| is required,
-  // |max_memory_cache_hints| is optional and the default max size will be used
-  // if it is not provided.
+  // Construct the HintCache with an optional backing store and max host-keyed
+  // cache size. If a backing store is not provided, all hints will only be
+  // stored in-memory.
   explicit HintCache(
       std::unique_ptr<OptimizationGuideStore> optimization_guide_store,
-      base::Optional<int> max_memory_cache_hints = base::Optional<int>());
+      int max_host_keyed_memory_cache_size);
   ~HintCache();
 
-  // Initializes the backing store contained within the hint cache and
-  // asynchronously runs the callback after initialization is complete.
-  // If |purge_existing_data| is set to true, then the cache will purge any
+  // Initializes the backing store contained within the hint cache, if provided,
+  // and asynchronously runs the callback after initialization is complete. If
+  // |purge_existing_data| is set to true, then the cache will purge any
   // pre-existing data and begin in a clean state.
   void Initialize(bool purge_existing_data, base::OnceClosure callback);
 

components/optimization_guide/hints_fetcher.cc

@@ -126,6 +126,11 @@ bool HintsFetcher::WasHostCoveredByFetch(PrefService* pref_service,
 bool HintsFetcher::WasHostCoveredByFetch(PrefService* pref_service,
                                          const std::string& host,
                                          const base::Clock* time_clock) {
+  if (!optimization_guide::features::ShouldPersistHintsToDisk()) {
+    // Don't consult the pref if we aren't even persisting hints to disk.
+    return false;
+  }
+
   DictionaryPrefUpdate hosts_fetched(
       pref_service, prefs::kHintsFetcherHostsSuccessfullyFetched);
   base::Optional<double> value =
@@ -325,6 +330,11 @@ void HintsFetcher::HandleResponse(const std::string& get_hints_response_data,
 
 void HintsFetcher::UpdateHostsSuccessfullyFetched(
     base::TimeDelta valid_duration) {
+  if (!optimization_guide::features::ShouldPersistHintsToDisk()) {
+    // Do not persist any state if we aren't persisting hints to disk.
+    return;
+  }
+
   DictionaryPrefUpdate hosts_fetched_list(
       pref_service_, prefs::kHintsFetcherHostsSuccessfullyFetched);
 
@@ -417,7 +427,7 @@ std::vector<std::string> HintsFetcher::GetSizeLimitedHostsDueForHintsRefresh(
 
     base::Optional<double> value =
         hosts_fetched->FindDoubleKey(HashHostForDictionary(host));
-    if (value) {
+    if (value && optimization_guide::features::ShouldPersistHintsToDisk()) {
       base::Time host_valid_time = base::Time::FromDeltaSinceWindowsEpoch(
           base::TimeDelta::FromSecondsD(*value));
       host_hints_due_for_refresh =

components/optimization_guide/optimization_guide_features.cc

@@ -242,6 +242,12 @@ size_t MaxHostModelFeaturesCacheSize() {
       kOptimizationTargetPrediction, "max_host_model_features_cache_size", 100);
 }
 
+size_t MaxHostKeyedHintCacheSize() {
+  size_t max_host_keyed_hint_cache_size = GetFieldTrialParamByFeatureAsInt(
+      kOptimizationHints, "max_host_keyed_hint_cache_size", 30);
+  return max_host_keyed_hint_cache_size;
+}
+
 size_t MaxURLKeyedHintCacheSize() {
   size_t max_url_keyed_hint_cache_size = GetFieldTrialParamByFeatureAsInt(
       kOptimizationHints, "max_url_keyed_hint_cache_size", 30);
@@ -250,6 +256,11 @@ size_t MaxURLKeyedHintCacheSize() {
   return max_url_keyed_hint_cache_size;
 }
 
+bool ShouldPersistHintsToDisk() {
+  return GetFieldTrialParamByFeatureAsBool(kOptimizationHints,
+                                           "persist_hints_to_disk", true);
+}
+
 bool ShouldOverrideOptimizationTargetDecisionForMetricsPurposes(
     proto::OptimizationTarget optimization_target) {
   if (optimization_target != proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD)

components/optimization_guide/optimization_guide_features.h

@@ -122,9 +122,17 @@ size_t MaxHostsForOptimizationGuideServiceModelsFetch();
 size_t MaxHostModelFeaturesCacheSize();
 
 // The maximum number of hints allowed to be maintained in a least-recently-used
-// cache.
+// cache for hosts.
+size_t MaxHostKeyedHintCacheSize();
+
+// The maximum number of hints allowed to be maintained in a least-recently-used
+// cache for URLs.
 size_t MaxURLKeyedHintCacheSize();
 
+// Returns true if hints should be persisted to disk. If this is false, hints
+// will just be stored in-memory and evicted if not recently used.
+bool ShouldPersistHintsToDisk();
+
 // Returns true if the optimization target decision for |optimization_target|
 // should not be propagated to the caller in an effort to fully understand the
 // statistics for the served model and not taint the resulting data.