diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 2576bb2db87e..f77360db61b3 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -15,6 +15,7 @@ #include "base/memory/raw_ptr.h" #include "base/path_service.h" #include "base/strings/stringprintf.h" +#include "base/test/metrics/histogram_tester.h" #include "base/test/scoped_feature_list.h" #include "base/test/thread_test_helper.h" #include "brave/browser/brave_browser_process.h" @@ -271,8 +272,7 @@ class EngineTestObserver : public brave_shields::AdBlockEngine::TestObserver { bool AdBlockServiceTest::InstallRegionalAdBlockExtension( const std::string& uuid, bool enable_list) { - // The regional adblock engines depend on the default engine for resource - // loads. + // Install the default engine first. EXPECT_TRUE(InstallDefaultAdBlockExtension()); auto* default_engine = g_brave_browser_process->ad_block_service()->default_engine_.get(); @@ -463,6 +463,36 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); } +class AdBlockServiceEngineUpdateCountTest : public AdBlockServiceTest { + protected: + const base::HistogramTester histogram_tester_; +}; + +// The test verifies the number of expected engine updating during normal +// startup. +// Warning: each engine updating is a CPU-heavy thing and degrades startup +// performance. +IN_PROC_BROWSER_TEST_F(AdBlockServiceEngineUpdateCountTest, + DefaultStartupWithCookieList) { + // The empty ruleset building until the components are loaded. + // TODO(matuchin): remove that excessive work. + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Default", 1); + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Additional", 1); + + // Loads the default list first, then the additional cookie list. + ASSERT_TRUE( + InstallRegionalAdBlockExtension(brave_shields::kCookieListUuid, true)); + + // Only one new rebuild is expected. Loading the extra list must not + // trigger another rebuilding of the default engine and vice versa. + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Default", 2); + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Additional", 2); +} + // Load a page with an ad image, and make sure it is blocked by the // regional blocker. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByRegionalBlocker) { diff --git a/components/brave_component_updater/browser/dat_file_util.cc b/components/brave_component_updater/browser/dat_file_util.cc index 9376d9db8850..ce8c9f55e8db 100644 --- a/components/brave_component_updater/browser/dat_file_util.cc +++ b/components/brave_component_updater/browser/dat_file_util.cc @@ -8,9 +8,10 @@ #include #include -#include "base/logging.h" #include "base/files/file_path.h" #include "base/files/file_util.h" +#include "base/logging.h" +#include "base/trace_event/trace_event.h" namespace { @@ -40,8 +41,11 @@ void GetDATFileData(const base::FilePath& file_path, namespace brave_component_updater { DATFileDataBuffer ReadDATFileData(const base::FilePath& dat_file_path) { + TRACE_EVENT_BEGIN1("brave.adblock", "ReadDATFileData", "path", + dat_file_path.MaybeAsASCII()); DATFileDataBuffer buffer; GetDATFileData(dat_file_path, &buffer); + TRACE_EVENT_END1("brave.adblock", "ReadDATFileData", "size", buffer.size()); return buffer; } diff --git a/components/brave_shields/browser/ad_block_component_filters_provider.cc b/components/brave_shields/browser/ad_block_component_filters_provider.cc index 9bd489ddf2ee..d6fe5c53dfa0 100644 --- a/components/brave_shields/browser/ad_block_component_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_component_filters_provider.cc @@ -66,7 +66,7 @@ void AdBlockComponentFiltersProvider::OnComponentReady( const base::FilePath& path) { component_path_ = path; - NotifyObservers(); + NotifyObservers(engine_is_default_); } void AdBlockComponentFiltersProvider::LoadDATBuffer( diff --git a/components/brave_shields/browser/ad_block_custom_filters_provider.cc b/components/brave_shields/browser/ad_block_custom_filters_provider.cc index 51e35dafa0fa..bbe8d5a8a795 100644 --- a/components/brave_shields/browser/ad_block_custom_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_custom_filters_provider.cc @@ -54,7 +54,7 @@ bool AdBlockCustomFiltersProvider::UpdateCustomFilters( return false; local_state_->SetString(prefs::kAdBlockCustomFilters, custom_filters); - NotifyObservers(); + NotifyObservers(engine_is_default_); return true; } @@ -78,7 +78,7 @@ void AdBlockCustomFiltersProvider::LoadDATBuffer( void AdBlockCustomFiltersProvider::AddObserver( AdBlockFiltersProvider::Observer* observer) { AdBlockFiltersProvider::AddObserver(observer); - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields diff --git a/components/brave_shields/browser/ad_block_engine.cc b/components/brave_shields/browser/ad_block_engine.cc index 5aa3f42b6551..a9b6c10690fc 100644 --- a/components/brave_shields/browser/ad_block_engine.cc +++ b/components/brave_shields/browser/ad_block_engine.cc @@ -12,7 +12,11 @@ #include "base/containers/contains.h" #include "base/json/json_reader.h" +#include "base/logging.h" +#include "base/metrics/histogram_functions.h" #include "base/strings/string_number_conversions.h" +#include "base/timer/elapsed_timer.h" +#include "base/trace_event/trace_event.h" #include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "url/origin.h" @@ -243,6 +247,7 @@ base::Value::List AdBlockEngine::HiddenClassIdSelectors( void AdBlockEngine::Load(bool deserialize, const DATFileDataBuffer& dat_buf, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); if (deserialize) { OnDATLoaded(dat_buf, resources_json); } else { @@ -275,7 +280,23 @@ void AdBlockEngine::AddKnownTagsToAdBlockInstance() { void AdBlockEngine::OnListSourceLoaded(const DATFileDataBuffer& filters, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + + base::ElapsedTimer timer; + TRACE_EVENT_BEGIN2("brave.adblock", "MakeEngineWithRules", "size", + filters.size(), "is_default_engine", is_default_engine_); + auto result = adblock::engine_with_rules(filters); + + TRACE_EVENT_END0("brave.adblock", "MakeEngineWithRules"); + if (is_default_engine_) { + base::UmaHistogramTimes("Brave.Adblock.MakeEngineWithRules.Default", + timer.Elapsed()); + } else { + base::UmaHistogramTimes("Brave.Adblock.MakeEngineWithRules.Additional", + timer.Elapsed()); + } + if (result.result_kind != adblock::ResultKind::Success) { LOG(ERROR) << "AdBlockEngine::OnListSourceLoaded failed: " << result.error_message.c_str(); @@ -286,13 +307,30 @@ void AdBlockEngine::OnListSourceLoaded(const DATFileDataBuffer& filters, void AdBlockEngine::OnDATLoaded(const DATFileDataBuffer& dat_buf, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + // An empty buffer will not load successfully. if (dat_buf.empty()) { return; } + base::ElapsedTimer timer; + TRACE_EVENT_BEGIN2("brave.adblock", "EngineDeserialize", "size", + dat_buf.size(), "is_default_engine", is_default_engine_); + auto client = adblock::new_engine(); - if (!client->deserialize(dat_buf)) { + const auto result = client->deserialize(dat_buf); + + TRACE_EVENT_END0("brave.adblock", "EngineDeserialize"); + if (is_default_engine_) { + base::UmaHistogramTimes("Brave.Adblock.EngineDeserialize.Default", + timer.Elapsed()); + } else { + base::UmaHistogramTimes("Brave.Adblock.EngineDeserialize.Additional", + timer.Elapsed()); + } + + if (!result) { LOG(ERROR) << "AdBlockEngine::OnDATLoaded deserialize failed"; return; } diff --git a/components/brave_shields/browser/ad_block_filters_provider.cc b/components/brave_shields/browser/ad_block_filters_provider.cc index 4c20c9d840eb..23f23dcbe34f 100644 --- a/components/brave_shields/browser/ad_block_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_filters_provider.cc @@ -36,9 +36,9 @@ void AdBlockFiltersProvider::RemoveObserver( observers_.RemoveObserver(observer); } -void AdBlockFiltersProvider::NotifyObservers() { +void AdBlockFiltersProvider::NotifyObservers(bool is_for_default_engine) { for (auto& observer : observers_) { - observer.OnChanged(); + observer.OnChanged(is_for_default_engine); } } diff --git a/components/brave_shields/browser/ad_block_filters_provider.h b/components/brave_shields/browser/ad_block_filters_provider.h index 479c4a55776d..8b67abc77bdb 100644 --- a/components/brave_shields/browser/ad_block_filters_provider.h +++ b/components/brave_shields/browser/ad_block_filters_provider.h @@ -24,7 +24,7 @@ class AdBlockFiltersProvider { public: class Observer : public base::CheckedObserver { public: - virtual void OnChanged() = 0; + virtual void OnChanged(bool is_for_default_engine) = 0; }; explicit AdBlockFiltersProvider(bool engine_is_default); @@ -50,7 +50,7 @@ class AdBlockFiltersProvider { virtual void LoadDATBuffer( base::OnceCallback) = 0; - void NotifyObservers(); + void NotifyObservers(bool is_for_default_engine); private: base::ObserverList observers_; diff --git a/components/brave_shields/browser/ad_block_filters_provider_manager.cc b/components/brave_shields/browser/ad_block_filters_provider_manager.cc index ce0379431bcd..397877a88294 100644 --- a/components/brave_shields/browser/ad_block_filters_provider_manager.cc +++ b/components/brave_shields/browser/ad_block_filters_provider_manager.cc @@ -63,15 +63,15 @@ void AdBlockFiltersProviderManager::RemoveProvider( auto it = filters_providers.find(provider); DCHECK(it != filters_providers.end()); filters_providers.erase(it); - NotifyObservers(); + NotifyObservers(is_for_default_engine); } std::string AdBlockFiltersProviderManager::GetNameForDebugging() { return "AdBlockCustomFiltersProvider"; } -void AdBlockFiltersProviderManager::OnChanged() { - NotifyObservers(); +void AdBlockFiltersProviderManager::OnChanged(bool is_for_default_engine) { + NotifyObservers(is_for_default_engine); } // Use LoadDATBufferForEngine instead, for Filter Provider Manager. diff --git a/components/brave_shields/browser/ad_block_filters_provider_manager.h b/components/brave_shields/browser/ad_block_filters_provider_manager.h index a74cbffba576..f6c4b554b199 100644 --- a/components/brave_shields/browser/ad_block_filters_provider_manager.h +++ b/components/brave_shields/browser/ad_block_filters_provider_manager.h @@ -51,7 +51,7 @@ class AdBlockFiltersProviderManager : public AdBlockFiltersProvider, const DATFileDataBuffer& dat_buf)> cb); // AdBlockFiltersProvider::Observer - void OnChanged() override; + void OnChanged(bool is_default_engine) override; void AddProvider(AdBlockFiltersProvider* provider, bool is_for_default_engine); diff --git a/components/brave_shields/browser/ad_block_localhost_filters_provider.cc b/components/brave_shields/browser/ad_block_localhost_filters_provider.cc index 37b34f130d28..d13c1994b765 100644 --- a/components/brave_shields/browser/ad_block_localhost_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_localhost_filters_provider.cc @@ -50,7 +50,7 @@ void AdBlockLocalhostFiltersProvider::LoadDATBuffer( void AdBlockLocalhostFiltersProvider::AddObserver( AdBlockFiltersProvider::Observer* observer) { AdBlockFiltersProvider::AddObserver(observer); - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields diff --git a/components/brave_shields/browser/ad_block_service.cc b/components/brave_shields/browser/ad_block_service.cc index d81bc9640569..9b1988eac0c3 100644 --- a/components/brave_shields/browser/ad_block_service.cc +++ b/components/brave_shields/browser/ad_block_service.cc @@ -96,7 +96,11 @@ AdBlockService::SourceProviderObserver::~SourceProviderObserver() { resource_provider_->RemoveObserver(this); } -void AdBlockService::SourceProviderObserver::OnChanged() { +void AdBlockService::SourceProviderObserver::OnChanged(bool is_default_engine) { + if (adblock_engine_->IsDefaultEngine() != is_default_engine) { + // Skip updates of another engine. + return; + } if (is_filter_provider_manager_) { static_cast(filters_provider_.get()) ->LoadDATBufferForEngine( diff --git a/components/brave_shields/browser/ad_block_service.h b/components/brave_shields/browser/ad_block_service.h index 4e2f79029e69..aea5b976f595 100644 --- a/components/brave_shields/browser/ad_block_service.h +++ b/components/brave_shields/browser/ad_block_service.h @@ -70,7 +70,7 @@ class AdBlockService { void OnDATLoaded(bool deserialize, const DATFileDataBuffer& dat_buf); // AdBlockFiltersProvider::Observer - void OnChanged() override; + void OnChanged(bool is_default_engine) override; // AdBlockResourceProvider::Observer void OnResourcesLoaded(const std::string& resources_json) override; diff --git a/components/brave_shields/browser/ad_block_subscription_filters_provider.cc b/components/brave_shields/browser/ad_block_subscription_filters_provider.cc index 67764923ec01..7b0d824754a9 100644 --- a/components/brave_shields/browser/ad_block_subscription_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_subscription_filters_provider.cc @@ -51,7 +51,7 @@ void AdBlockSubscriptionFiltersProvider::OnDATFileDataReady( } void AdBlockSubscriptionFiltersProvider::OnListAvailable() { - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields