Add additional layer of checks when clearing hints for new opt types.
The original check for clearing hints when a new optimization type is registered the first time is racy because of when register is called by OptimizationGuide clients. This change adds additional logic to either clear if the store is available when the type is registered or defers until the store becomes available. This also changes the first run experience logic to only clear hints that are also persisted to disk, which currently is only host-keyed hints. Bug: 1068754 Change-Id: I1bc2df217c178b4ef257380c3efd04740a07468b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2252478 Commit-Queue: Michael Crouse <mcrouse@chromium.org> Auto-Submit: Michael Crouse <mcrouse@chromium.org> Reviewed-by: Sophie Chang <sophiechang@chromium.org> Cr-Commit-Position: refs/heads/master@{#780373}
This commit is contained in:

committed by
Commit Bot

parent
fb4bb567cd
commit
77aa363d1e
chrome/browser/optimization_guide
components/optimization_guide
@ -1412,7 +1412,6 @@ IN_PROC_BROWSER_TEST_F(
|
||||
// should be recorded as covered by the hints fetcher.
|
||||
base::flat_set<std::string> expected_request;
|
||||
expected_request.insert(GURL(full_url).host());
|
||||
expected_request.insert(GURL(full_url).spec());
|
||||
SetExpectedHintsRequestForHostsAndUrls(expected_request);
|
||||
ui_test_utils::NavigateToURL(browser(), GURL(full_url));
|
||||
|
||||
@ -1426,8 +1425,8 @@ IN_PROC_BROWSER_TEST_F(
|
||||
histogram_tester->ExpectBucketCount(
|
||||
"OptimizationGuide.HintsManager.RaceNavigationFetchAttemptStatus",
|
||||
optimization_guide::RaceNavigationFetchAttemptStatus::
|
||||
kRaceNavigationFetchHostAndURL,
|
||||
2);
|
||||
kRaceNavigationFetchHost,
|
||||
1);
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -9,6 +9,7 @@
|
||||
|
||||
#include "base/bind.h"
|
||||
#include "base/bind_helpers.h"
|
||||
#include "base/command_line.h"
|
||||
#include "base/logging.h"
|
||||
#include "base/metrics/histogram_functions.h"
|
||||
#include "base/metrics/histogram_macros.h"
|
||||
@ -487,6 +488,13 @@ void OptimizationGuideHintsManager::OnHintCacheInitialized() {
|
||||
}
|
||||
}
|
||||
|
||||
// If the store is available, clear all hint state so newly registered types
|
||||
// can have their hints immediately included in hint fetches.
|
||||
if (hint_cache_->IsHintStoreAvailable() && should_clear_hints_for_new_type_) {
|
||||
ClearHostKeyedHints();
|
||||
should_clear_hints_for_new_type_ = false;
|
||||
}
|
||||
|
||||
// Register as an observer regardless of hint proto override usage. This is
|
||||
// needed as a signal during testing.
|
||||
optimization_guide_service_->AddObserver(this);
|
||||
@ -868,7 +876,6 @@ void OptimizationGuideHintsManager::RegisterOptimizationTypes(
|
||||
const std::vector<optimization_guide::proto::OptimizationType>&
|
||||
optimization_types) {
|
||||
bool should_load_new_optimization_filter = false;
|
||||
bool should_clear_hints_for_new_type = false;
|
||||
|
||||
DictionaryPrefUpdate previously_registered_opt_types(
|
||||
pref_service_,
|
||||
@ -887,7 +894,7 @@ void OptimizationGuideHintsManager::RegisterOptimizationTypes(
|
||||
optimization_guide::proto::OptimizationType_Name(optimization_type));
|
||||
if (!value) {
|
||||
if (!ShouldIgnoreNewlyRegisteredOptimizationType(optimization_type))
|
||||
should_clear_hints_for_new_type = true;
|
||||
should_clear_hints_for_new_type_ = true;
|
||||
previously_registered_opt_types->SetBoolKey(
|
||||
optimization_guide::proto::OptimizationType_Name(optimization_type),
|
||||
true);
|
||||
@ -902,10 +909,12 @@ void OptimizationGuideHintsManager::RegisterOptimizationTypes(
|
||||
}
|
||||
}
|
||||
|
||||
// Clear all hint state so newly registered types can have their hints
|
||||
// immediately included in hint fetches.
|
||||
if (should_clear_hints_for_new_type)
|
||||
ClearFetchedHints();
|
||||
// If the store is available, clear all hint state so newly registered types
|
||||
// can have their hints immediately included in hint fetches.
|
||||
if (hint_cache_->IsHintStoreAvailable() && should_clear_hints_for_new_type_) {
|
||||
ClearHostKeyedHints();
|
||||
should_clear_hints_for_new_type_ = false;
|
||||
}
|
||||
|
||||
if (should_load_new_optimization_filter) {
|
||||
if (optimization_guide::switches::IsHintComponentProcessingDisabled()) {
|
||||
@ -1351,6 +1360,12 @@ void OptimizationGuideHintsManager::ClearFetchedHints() {
|
||||
pref_service_);
|
||||
}
|
||||
|
||||
void OptimizationGuideHintsManager::ClearHostKeyedHints() {
|
||||
hint_cache_->ClearHostKeyedHints();
|
||||
optimization_guide::HintsFetcher::ClearHostsSuccessfullyFetched(
|
||||
pref_service_);
|
||||
}
|
||||
|
||||
void OptimizationGuideHintsManager::AddHintForTesting(
|
||||
const GURL& url,
|
||||
optimization_guide::proto::OptimizationType optimization_type,
|
||||
|
@ -134,9 +134,13 @@ class OptimizationGuideHintsManager
|
||||
optimization_guide::proto::OptimizationType optimization_type,
|
||||
optimization_guide::OptimizationGuideDecisionCallback callback);
|
||||
|
||||
// Clears fetched hints from |hint_cache_|.
|
||||
// Clears all fetched hints from |hint_cache_|.
|
||||
void ClearFetchedHints();
|
||||
|
||||
// Clears the host-keyed fetched hints from |hint_cache_|, both the persisted
|
||||
// and in memory ones.
|
||||
void ClearHostKeyedHints();
|
||||
|
||||
// Returns the current batch update hints fetcher.
|
||||
optimization_guide::HintsFetcher* batch_update_hints_fetcher() const {
|
||||
return batch_update_hints_fetcher_.get();
|
||||
@ -460,6 +464,10 @@ class OptimizationGuideHintsManager
|
||||
// Service.
|
||||
const base::Clock* clock_;
|
||||
|
||||
// Whether fetched hints should be cleared when the store is initialized
|
||||
// because a new optimization type was registered.
|
||||
bool should_clear_hints_for_new_type_ = false;
|
||||
|
||||
// The current estimate of the EffectiveConnectionType.
|
||||
net::EffectiveConnectionType current_effective_connection_type_ =
|
||||
net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
|
||||
|
@ -106,10 +106,14 @@ void HintCache::PurgeExpiredFetchedHints() {
|
||||
void HintCache::ClearFetchedHints() {
|
||||
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
|
||||
DCHECK(optimization_guide_store_);
|
||||
// TODO(mcrouse): Update to remove only fetched hints from
|
||||
// |host_keyed_cache_|.
|
||||
host_keyed_cache_.Clear();
|
||||
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();
|
||||
}
|
||||
|
||||
@ -312,4 +316,10 @@ 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();
|
||||
}
|
||||
|
||||
} // namespace optimization_guide
|
||||
|
@ -89,9 +89,13 @@ class HintCache {
|
||||
void PurgeExpiredFetchedHints();
|
||||
|
||||
// Purges fetched hints from the owned |optimization_guide_store_| and resets
|
||||
// the host-keyed cache.
|
||||
// both in-memory hint caches.
|
||||
void ClearFetchedHints();
|
||||
|
||||
// Purges fetched hints from the owned |optimization_guide_store_| and resets
|
||||
// the host-keyed cache.
|
||||
void ClearHostKeyedHints();
|
||||
|
||||
// Returns whether the cache has a hint data for |host| locally (whether
|
||||
// in the host-keyed cache or persisted on disk).
|
||||
bool HasHint(const std::string& host) const;
|
||||
@ -130,6 +134,9 @@ class HintCache {
|
||||
google::protobuf::RepeatedPtrField<proto::Hint>* hints,
|
||||
StoreUpdateData* update_data);
|
||||
|
||||
// Returns whether the persistent hint store owned by this is available.
|
||||
bool IsHintStoreAvailable() const;
|
||||
|
||||
// Override |clock_| for testing.
|
||||
void SetClockForTesting(const base::Clock* clock);
|
||||
|
||||
|
@ -253,6 +253,9 @@ class OptimizationGuideStore {
|
||||
// Clears all host model features from the database and resets the entry keys.
|
||||
void ClearHostModelFeaturesFromDatabase();
|
||||
|
||||
// Returns true if the current status is Status::kAvailable.
|
||||
bool IsAvailable() const;
|
||||
|
||||
private:
|
||||
friend class OptimizationGuideStoreTest;
|
||||
friend class StoreUpdateData;
|
||||
@ -312,9 +315,6 @@ class OptimizationGuideStore {
|
||||
// transitions to Status::kFailed.
|
||||
void UpdateStatus(Status new_status);
|
||||
|
||||
// Returns true if the current status is Status::kAvailable.
|
||||
bool IsAvailable() const;
|
||||
|
||||
// Asynchronously purges all existing entries from the database and runs the
|
||||
// callback after it completes. This should only be run during initialization.
|
||||
void PurgeDatabase(base::OnceClosure callback);
|
||||
|
Reference in New Issue
Block a user