From d44dea5c1bcee567b7fc66d7f0d477eee1b7bca5 Mon Sep 17 00:00:00 2001 From: Khalid Peer Date: Thu, 12 Jan 2023 21:47:41 +0000 Subject: [PATCH] [Merge 110][ZPS] Enable cross-session persistence for ZPS on NTP. The current implementation of the in-memory ZPS cache is unable to persist cached entries across multiple user sessions due to storing entries entirely in memory. As such, when the user restarts their browser and/or starts a new session from scratch, it's possible for them to be presented with zero remote ZPS suggestions when focusing the omnibox (e.g. on a slow internet connection). In order to address this edge-case, this CL enhances the in-memory ZPS caching service with the ability to persist cached entries by means of the user preferences system which stores data on disk. Furthermore, this CL introduces a dedicated cache entry for storing ZPS responses received on the NTP (as opposed to simply caching them alongside ZPS responses for SRP/Web). This change will ensure that the NTP-related entry will no longer be evicted out of the cache due to a large number of SRP/Web-related cache entries. (cherry picked from commit bafe6e6bd92b4fde32aac397f80cf213abf88cf8) Bug: 1351224 Change-Id: Ia3a697e2487c37f48b5ad4e8e3a55bd5d2147bde Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4148352 Reviewed-by: Robbie Gibson Reviewed-by: Sophie Chang Reviewed-by: Moe Ahmadi Commit-Queue: Khalid Peer Cr-Original-Commit-Position: refs/heads/main@{#1090655} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4156972 Cr-Commit-Position: refs/branch-heads/5481@{#251} Cr-Branched-From: 130f3e4d850f4bc7387cfb8d08aa993d288a67a9-refs/heads/main@{#1084008} --- .../zero_suggest_cache_service_factory.cc | 4 +- .../browser/zero_suggest_cache_service.cc | 45 ++++++-- .../browser/zero_suggest_cache_service.h | 9 +- .../zero_suggest_cache_service_unittest.cc | 101 ++++++++++++++---- .../browser/zero_suggest_provider_unittest.cc | 11 +- .../content/browser/BUILD.gn | 3 + .../optimization_guide/content/browser/DEPS | 2 + ...otations_web_contents_observer_unittest.cc | 14 +++ .../zero_suggest_cache_service_factory.cc | 3 + 9 files changed, 159 insertions(+), 33 deletions(-) diff --git a/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc b/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc index 16d50cdba2c6ba..2c5dcf853d1421 100644 --- a/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc +++ b/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc @@ -4,6 +4,7 @@ #include "chrome/browser/autocomplete/zero_suggest_cache_service_factory.h" +#include "chrome/browser/profiles/profile.h" #include "components/omnibox/browser/omnibox_field_trial.h" // static @@ -20,8 +21,9 @@ ZeroSuggestCacheServiceFactory* ZeroSuggestCacheServiceFactory::GetInstance() { KeyedService* ZeroSuggestCacheServiceFactory::BuildServiceInstanceFor( content::BrowserContext* context) const { + Profile* profile = Profile::FromBrowserContext(context); return new ZeroSuggestCacheService( - OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get()); + profile->GetPrefs(), OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get()); } ZeroSuggestCacheServiceFactory::ZeroSuggestCacheServiceFactory() diff --git a/components/omnibox/browser/zero_suggest_cache_service.cc b/components/omnibox/browser/zero_suggest_cache_service.cc index 7d556de2c06ea5..705140b7360e3d 100644 --- a/components/omnibox/browser/zero_suggest_cache_service.cc +++ b/components/omnibox/browser/zero_suggest_cache_service.cc @@ -8,17 +8,41 @@ #include "base/trace_event/memory_usage_estimator.h" #include "components/omnibox/browser/autocomplete_input.h" #include "components/omnibox/browser/autocomplete_provider_client.h" +#include "components/omnibox/browser/omnibox_prefs.h" #include "components/omnibox/browser/search_suggestion_parser.h" +#include "components/prefs/pref_service.h" using CacheEntry = ZeroSuggestCacheService::CacheEntry; -ZeroSuggestCacheService::ZeroSuggestCacheService(size_t cache_size) - : cache_(cache_size) {} +ZeroSuggestCacheService::ZeroSuggestCacheService(PrefService* prefs, + size_t cache_size) + : prefs_(prefs), cache_(cache_size) { + DCHECK_GT(cache_size, 0u); -ZeroSuggestCacheService::~ZeroSuggestCacheService() = default; + if (prefs_) { + // Load cached ZPS response for NTP from prior session via prefs. + ntp_entry_.response_json = + omnibox::GetUserPreferenceForZeroSuggestCachedResponse(prefs_, + /*page_url=*/""); + } +} + +ZeroSuggestCacheService::~ZeroSuggestCacheService() { + if (prefs_) { + // Dump cached ZPS response for NTP to prefs. + omnibox::SetUserPreferenceForZeroSuggestCachedResponse( + prefs_, /*page_url=*/"", /*response=*/ntp_entry_.response_json); + } +} CacheEntry ZeroSuggestCacheService::ReadZeroSuggestResponse( const std::string& page_url) const { + // Read cached ZPS response for NTP. + if (page_url.empty()) { + return ntp_entry_; + } + + // Read cached ZPS response for SRP/Web. const auto it = cache_.Get(page_url); return it != cache_.end() ? it->second : CacheEntry(); } @@ -26,9 +50,18 @@ CacheEntry ZeroSuggestCacheService::ReadZeroSuggestResponse( void ZeroSuggestCacheService::StoreZeroSuggestResponse( const std::string& page_url, const CacheEntry& response) { - cache_.Put(page_url, response); - base::UmaHistogramCounts1M("Omnibox.ZeroSuggestProvider.CacheMemoryUsage", - base::trace_event::EstimateMemoryUsage(cache_)); + if (page_url.empty()) { + // Write ZPS response for NTP to cache. + ntp_entry_ = response; + } else { + // Write ZPS response for SRP/Web to cache. + cache_.Put(page_url, response); + } + + base::UmaHistogramCounts1M( + "Omnibox.ZeroSuggestProvider.CacheMemoryUsage", + base::trace_event::EstimateMemoryUsage(cache_) + + base::trace_event::EstimateMemoryUsage(ntp_entry_)); for (auto& observer : observers_) { observer.OnZeroSuggestResponseUpdated(page_url, response); diff --git a/components/omnibox/browser/zero_suggest_cache_service.h b/components/omnibox/browser/zero_suggest_cache_service.h index 812346ce0813bc..30165936c85dd0 100644 --- a/components/omnibox/browser/zero_suggest_cache_service.h +++ b/components/omnibox/browser/zero_suggest_cache_service.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_OMNIBOX_BROWSER_ZERO_SUGGEST_CACHE_SERVICE_H_ #define COMPONENTS_OMNIBOX_BROWSER_ZERO_SUGGEST_CACHE_SERVICE_H_ +#include #include #include "base/containers/lru_cache.h" @@ -15,6 +16,7 @@ #include "components/omnibox/browser/autocomplete_input.h" #include "components/omnibox/browser/autocomplete_provider_client.h" #include "components/omnibox/browser/search_suggestion_parser.h" +#include "components/prefs/pref_service.h" class ZeroSuggestCacheService : public KeyedService { public: @@ -50,7 +52,7 @@ class ZeroSuggestCacheService : public KeyedService { const CacheEntry& response) {} }; - explicit ZeroSuggestCacheService(size_t cache_size); + ZeroSuggestCacheService(PrefService* prefs, size_t cache_size); ZeroSuggestCacheService(const ZeroSuggestCacheService&) = delete; ZeroSuggestCacheService& operator=(const ZeroSuggestCacheService&) = delete; @@ -73,11 +75,16 @@ class ZeroSuggestCacheService : public KeyedService { void RemoveObserver(Observer* observer); private: + // Pref service used for in-memory cache data persistence. Not owned. + const raw_ptr prefs_; // Cache mapping each page URL to the corresponding zero suggest response // (serialized JSON). |mutable| is used here because reading from the cache, // while logically const, will actually modify the internal recency list of // the HashingLRUCache object. mutable base::HashingLRUCache cache_; + // Dedicated cache entry for "ZPS on NTP" data in order to minimize any + // negative impact due to cache eviction policy. + CacheEntry ntp_entry_; base::ObserverList observers_; }; diff --git a/components/omnibox/browser/zero_suggest_cache_service_unittest.cc b/components/omnibox/browser/zero_suggest_cache_service_unittest.cc index 7944a9ebe49c15..baf15a3103fbe4 100644 --- a/components/omnibox/browser/zero_suggest_cache_service_unittest.cc +++ b/components/omnibox/browser/zero_suggest_cache_service_unittest.cc @@ -8,6 +8,9 @@ #include #include "base/test/metrics/histogram_tester.h" +#include "components/omnibox/browser/omnibox_prefs.h" +#include "components/omnibox/browser/zero_suggest_provider.h" +#include "components/prefs/testing_pref_service.h" #include "testing/gtest/include/gtest/gtest.h" using CacheEntry = ZeroSuggestCacheService::CacheEntry; @@ -37,21 +40,42 @@ struct TestCacheEntry { std::string response; }; -TEST(ZeroSuggestCacheServiceTest, CacheStartsEmpty) { - ZeroSuggestCacheService cache_svc(1); +class ZeroSuggestCacheServiceTest : public testing::Test { + public: + ZeroSuggestCacheServiceTest() = default; + + ZeroSuggestCacheServiceTest(const ZeroSuggestCacheServiceTest&) = delete; + ZeroSuggestCacheServiceTest& operator=(const ZeroSuggestCacheServiceTest&) = + delete; + + void SetUp() override { + prefs_ = std::make_unique(); + ZeroSuggestProvider::RegisterProfilePrefs(prefs_->registry()); + } + + void TearDown() override { prefs_.reset(); } + + PrefService* GetPrefs() { return prefs_.get(); } + + private: + std::unique_ptr prefs_; +}; + +TEST_F(ZeroSuggestCacheServiceTest, CacheStartsEmpty) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); EXPECT_TRUE(cache_svc.IsCacheEmpty()); } -TEST(ZeroSuggestCacheServiceTest, StoreResponsePopulatesCache) { - ZeroSuggestCacheService cache_svc(1); +TEST_F(ZeroSuggestCacheServiceTest, StoreResponsePopulatesCache) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); cache_svc.StoreZeroSuggestResponse("https://www.google.com", CacheEntry("foo")); EXPECT_FALSE(cache_svc.IsCacheEmpty()); } -TEST(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) { +TEST_F(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) { base::HistogramTester histogram_tester; - ZeroSuggestCacheService cache_svc(1); + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); const std::string page_url = "https://www.google.com"; const std::string response = "foo"; @@ -75,8 +99,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) { histogram_tester.ExpectTotalCount(histogram, 4); } -TEST(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) { - ZeroSuggestCacheService cache_svc(1); +TEST_F(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); const std::string page_url = "https://www.google.com"; const std::string old_response = "foo"; @@ -91,8 +115,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) { new_response); } -TEST(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) { - ZeroSuggestCacheService cache_svc(2); +TEST_F(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 2); const std::string goog_url = "https://www.google.com"; const std::string fb_url = "https://www.facebook.com"; @@ -142,8 +166,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) { EXPECT_EQ(fb_observer.GetData().response_json, "bar"); } -TEST(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) { - ZeroSuggestCacheService cache_svc(2); +TEST_F(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 2); TestCacheEntry entry1 = {"https://www.facebook.com", "foo"}; TestCacheEntry entry2 = {"https://www.google.com", "bar"}; @@ -168,8 +192,8 @@ TEST(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) { entry3.response); } -TEST(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) { - ZeroSuggestCacheService cache_svc(1); +TEST_F(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); const std::string page_url = "https://www.google.com"; const std::string response = "foo"; @@ -179,8 +203,8 @@ TEST(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) { response); } -TEST(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) { - ZeroSuggestCacheService cache_svc(2); +TEST_F(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 2); TestCacheEntry entry1 = {"https://www.google.com", "foo"}; TestCacheEntry entry2 = {"https://www.facebook.com", "bar"}; @@ -206,13 +230,54 @@ TEST(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) { entry3.response); } -TEST(ZeroSuggestCacheServiceTest, ClearCacheResultsInEmptyCache) { - ZeroSuggestCacheService cache_svc(1); +TEST_F(ZeroSuggestCacheServiceTest, ClearCacheResultsInEmptyCache) { + ZeroSuggestCacheService cache_svc(GetPrefs(), 1); cache_svc.StoreZeroSuggestResponse("https://www.google.com", CacheEntry("foo")); + EXPECT_FALSE(cache_svc.IsCacheEmpty()); + cache_svc.ClearCache(); EXPECT_TRUE(cache_svc.IsCacheEmpty()); } + +TEST_F(ZeroSuggestCacheServiceTest, CacheLoadsFromPrefsOnStartup) { + TestCacheEntry ntp_entry = {"", "foo"}; + + PrefService* prefs = GetPrefs(); + omnibox::SetUserPreferenceForZeroSuggestCachedResponse(prefs, ntp_entry.url, + ntp_entry.response); + ZeroSuggestCacheService cache_svc(prefs, 1); + + EXPECT_EQ(cache_svc.ReadZeroSuggestResponse(ntp_entry.url).response_json, + ntp_entry.response); +} + +TEST_F(ZeroSuggestCacheServiceTest, CacheDumpsToPrefsOnShutdown) { + TestCacheEntry ntp_entry = {"", "foo"}; + + PrefService* prefs = GetPrefs(); + { + ZeroSuggestCacheService cache_svc(prefs, 1); + cache_svc.StoreZeroSuggestResponse(ntp_entry.url, + CacheEntry(ntp_entry.response)); + } + + EXPECT_EQ(omnibox::GetUserPreferenceForZeroSuggestCachedResponse( + prefs, ntp_entry.url), + ntp_entry.response); +} + +TEST_F(ZeroSuggestCacheServiceTest, CacheWorksGivenNullPrefService) { + TestCacheEntry ntp_entry = {"", "foo"}; + + PrefService* prefs = nullptr; + ZeroSuggestCacheService cache_svc(prefs, 1); + cache_svc.StoreZeroSuggestResponse(ntp_entry.url, + CacheEntry(ntp_entry.response)); + + EXPECT_EQ(cache_svc.ReadZeroSuggestResponse(ntp_entry.url).response_json, + ntp_entry.response); +} diff --git a/components/omnibox/browser/zero_suggest_provider_unittest.cc b/components/omnibox/browser/zero_suggest_provider_unittest.cc index 8375c213d2719e..ec0beb18243e58 100644 --- a/components/omnibox/browser/zero_suggest_provider_unittest.cc +++ b/components/omnibox/browser/zero_suggest_provider_unittest.cc @@ -52,13 +52,10 @@ class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient { public: FakeAutocompleteProviderClient() : template_url_service_(new TemplateURLService(nullptr, 0)), - pref_service_(new TestingPrefServiceSimple()), - zero_suggest_cache_service_( - std::make_unique(kCacheSize)) { - pref_service_->registry()->RegisterStringPref( - omnibox::kZeroSuggestCachedResults, std::string()); - pref_service_->registry()->RegisterDictionaryPref( - omnibox::kZeroSuggestCachedResultsWithURL, base::Value::Dict()); + pref_service_(new TestingPrefServiceSimple()) { + ZeroSuggestProvider::RegisterProfilePrefs(pref_service_->registry()); + zero_suggest_cache_service_ = std::make_unique( + pref_service_.get(), kCacheSize); } FakeAutocompleteProviderClient(const FakeAutocompleteProviderClient&) = delete; diff --git a/components/optimization_guide/content/browser/BUILD.gn b/components/optimization_guide/content/browser/BUILD.gn index 82c0a518c6c69c..14a8cd91f326fd 100644 --- a/components/optimization_guide/content/browser/BUILD.gn +++ b/components/optimization_guide/content/browser/BUILD.gn @@ -95,7 +95,10 @@ source_set("unit_tests") { deps = [ ":browser", ":test_support", + "//components/history/core/test", + "//components/omnibox/browser:test_support", "//components/optimization_guide/core:test_support", + "//components/prefs:test_support", "//components/search_engines", "//components/ukm:test_support", "//content/test:test_support", diff --git a/components/optimization_guide/content/browser/DEPS b/components/optimization_guide/content/browser/DEPS index 9da827c4dd8853..5deccd69a8fca1 100644 --- a/components/optimization_guide/content/browser/DEPS +++ b/components/optimization_guide/content/browser/DEPS @@ -3,9 +3,11 @@ include_rules = [ "+components/continuous_search/common/public/mojom", "+components/google/core", "+components/history/core/browser", + "+components/history/core/test", "+components/keyed_service/core", "+components/no_state_prefetch/browser", "+components/omnibox/browser", + "+components/omnibox/common", "+components/optimization_guide/core", "+components/optimization_guide/proto", "+components/search_engines", diff --git a/components/optimization_guide/content/browser/page_content_annotations_web_contents_observer_unittest.cc b/components/optimization_guide/content/browser/page_content_annotations_web_contents_observer_unittest.cc index 1af638387384e8..bc62afa21dacc0 100644 --- a/components/optimization_guide/content/browser/page_content_annotations_web_contents_observer_unittest.cc +++ b/components/optimization_guide/content/browser/page_content_annotations_web_contents_observer_unittest.cc @@ -10,11 +10,18 @@ #include "base/test/scoped_feature_list.h" #include "components/google/core/common/google_switches.h" #include "components/history/core/browser/history_service.h" +#include "components/history/core/browser/history_types.h" +#include "components/history/core/test/test_history_database.h" +#include "components/omnibox/browser/fake_autocomplete_provider_client.h" +#include "components/omnibox/browser/zero_suggest_cache_service.h" +#include "components/omnibox/browser/zero_suggest_provider.h" +#include "components/omnibox/common/omnibox_features.h" #include "components/optimization_guide/content/browser/page_content_annotations_service.h" #include "components/optimization_guide/content/browser/test_optimization_guide_decider.h" #include "components/optimization_guide/core/optimization_guide_features.h" #include "components/optimization_guide/core/test_optimization_guide_model_provider.h" #include "components/optimization_guide/proto/page_entities_metadata.pb.h" +#include "components/prefs/testing_pref_service.h" #include "components/search_engines/template_url_service.h" #include "components/ukm/test_ukm_recorder.h" #include "content/public/browser/navigation_handle.h" @@ -194,6 +201,10 @@ class PageContentAnnotationsWebContentsObserverTest optimization_guide_model_provider_ = std::make_unique(); history_service_ = std::make_unique(); + pref_service_ = std::make_unique(); + ZeroSuggestProvider::RegisterProfilePrefs(pref_service_->registry()); + zero_suggest_cache_service_ = std::make_unique( + pref_service_.get(), /*cache_size=*/1); page_content_annotations_service_ = std::make_unique( optimization_guide_model_provider_.get(), history_service_.get()); @@ -249,6 +260,9 @@ class PageContentAnnotationsWebContentsObserverTest std::unique_ptr optimization_guide_model_provider_; std::unique_ptr history_service_; + base::ScopedTempDir temp_dir_; + std::unique_ptr pref_service_; + std::unique_ptr zero_suggest_cache_service_; std::unique_ptr page_content_annotations_service_; std::unique_ptr template_url_service_; diff --git a/ios/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc b/ios/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc index 35e9f805ef46ff..f691c3213daa8b 100644 --- a/ios/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc +++ b/ios/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc @@ -35,7 +35,10 @@ ZeroSuggestCacheServiceFactory::~ZeroSuggestCacheServiceFactory() = default; std::unique_ptr ZeroSuggestCacheServiceFactory::BuildServiceInstanceFor( web::BrowserState* context) const { + ChromeBrowserState* browser_state = + ChromeBrowserState::FromBrowserState(context); return std::make_unique( + browser_state->GetPrefs(), OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get()); }