Emit deduplication metrics on main thread
Currently, deduplication metrics are emitted on a background thread at
startup, since their computation is O(#profiles^2). This caused the
rollout to fail due to thread safety issues [1].
This CL removes the logic that posts the computation on a background
thread.
Since the code is behind the AutofillLogDeduplicationMetrics feature
flag, let's see if this causes any startup time regressions. Since most
users have very few profiles, this might actually be fine.
[1] https://docs.google.com/document/d/1_IjAlPJrAsiKslCfgeXZqGk2MMuH9UlImoXEa1UlbUE/edit?tab=t.0
Bug: 325452461
Change-Id: I670394e58d8b813187f46d75af45d356b5b1b90c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5691472
Reviewed-by: Jihad Hanna <jihadghanna@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Florian Leimgruber <fleimgruber@google.com>
Cr-Commit-Position: refs/heads/main@{#1325427}
diff --git a/components/autofill/core/browser/metrics/profile_deduplication_metrics.cc b/components/autofill/core/browser/metrics/profile_deduplication_metrics.cc
index f88b6d7..c3a36a21 100644
--- a/components/autofill/core/browser/metrics/profile_deduplication_metrics.cc
+++ b/components/autofill/core/browser/metrics/profile_deduplication_metrics.cc
@@ -16,7 +16,6 @@
#include "base/metrics/histogram_functions.h"
#include "base/strings/strcat.h"
#include "base/strings/string_number_conversions.h"
-#include "base/task/thread_pool.h"
#include "components/autofill/core/browser/address_data_cleaner.h"
#include "components/autofill/core/browser/data_model/autofill_profile.h"
#include "components/autofill/core/browser/data_model/autofill_profile_comparator.h"
@@ -80,26 +79,12 @@
// Don't pollute metrics with cases where obviously no duplicates exists.
return;
}
- auto log_metrics = [](std::vector<AutofillProfile> profiles,
- const std::string& app_locale) {
- AutofillProfileComparator comparator(app_locale);
- for (AutofillProfile& profile : profiles) {
- LogDeduplicationStartupMetricsForProfile(
- profile, AddressDataCleaner::CalculateMinimalIncompatibleTypeSets(
- profile, profiles, comparator));
- }
- };
- // Since computing the metrics is quadratic in `profiles.size()`, it is done
- // on a background thread. Create a copy of the `profiles`, to avoid passing
- // pointers between threads.
- std::vector<AutofillProfile> profiles_copy;
- profiles_copy.reserve(profiles.size());
+ AutofillProfileComparator comparator(app_locale);
for (const AutofillProfile* profile : profiles) {
- profiles_copy.push_back(*profile);
+ LogDeduplicationStartupMetricsForProfile(
+ *profile, AddressDataCleaner::CalculateMinimalIncompatibleTypeSets(
+ *profile, profiles, comparator));
}
- base::ThreadPool::PostTask(
- FROM_HERE, base::BindOnce(log_metrics, std::move(profiles_copy),
- std::string(app_locale)));
}
void LogDeduplicationImportMetrics(
diff --git a/components/autofill/core/browser/metrics/profile_deduplication_metrics_unittest.cc b/components/autofill/core/browser/metrics/profile_deduplication_metrics_unittest.cc
index 354660f..f723348 100644
--- a/components/autofill/core/browser/metrics/profile_deduplication_metrics_unittest.cc
+++ b/components/autofill/core/browser/metrics/profile_deduplication_metrics_unittest.cc
@@ -5,7 +5,6 @@
#include "components/autofill/core/browser/metrics/profile_deduplication_metrics.h"
#include "base/test/metrics/histogram_tester.h"
-#include "base/test/task_environment.h"
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/data_model/autofill_profile.h"
#include "components/autofill/core/browser/field_types.h"
@@ -19,17 +18,8 @@
constexpr char kLocale[] = "en_US";
-class ProfileDeduplicationMetricsTest : public testing::Test {
- public:
- // Startup metrics are computed asynchronously. Used to wait for the result.
- void RunUntilIdle() { task_environment_.RunUntilIdle(); }
-
- private:
- base::test::TaskEnvironment task_environment_;
-};
-
-TEST_F(ProfileDeduplicationMetricsTest,
- Startup_RankOfStoredQuasiDuplicateProfiles) {
+TEST(ProfileDeduplicationMetricsTest,
+ Startup_RankOfStoredQuasiDuplicateProfiles) {
// Create a pair of profiles with duplication rank 2.
AutofillProfile a = test::GetFullProfile();
AutofillProfile b = test::GetFullProfile();
@@ -38,7 +28,6 @@
base::HistogramTester histogram_tester;
const std::vector<AutofillProfile*> profiles = {&a, &b};
LogDeduplicationStartupMetrics(profiles, kLocale);
- RunUntilIdle();
// The same sample is emitted once for each profile.
histogram_tester.ExpectUniqueSample(
"Autofill.Deduplication.ExistingProfiles."
@@ -47,20 +36,19 @@
}
// Tests that when the user doesn't have other profiles, no metrics are emitted.
-TEST_F(ProfileDeduplicationMetricsTest,
- Startup_RankOfStoredQuasiDuplicateProfiles_NoProfiles) {
+TEST(ProfileDeduplicationMetricsTest,
+ Startup_RankOfStoredQuasiDuplicateProfiles_NoProfiles) {
AutofillProfile a = test::GetFullProfile();
base::HistogramTester histogram_tester;
const std::vector<AutofillProfile*> profiles = {&a};
LogDeduplicationStartupMetrics(profiles, kLocale);
- RunUntilIdle();
histogram_tester.ExpectTotalCount(
"Autofill.Deduplication.ExistingProfiles."
"RankOfStoredQuasiDuplicateProfiles",
0);
}
-TEST_F(ProfileDeduplicationMetricsTest, Startup_TypeOfQuasiDuplicateToken) {
+TEST(ProfileDeduplicationMetricsTest, Startup_TypeOfQuasiDuplicateToken) {
// `a` differs from `b` and `c` only in a single type.
AutofillProfile a = test::GetFullProfile();
AutofillProfile b = test::GetFullProfile();
@@ -70,7 +58,6 @@
base::HistogramTester histogram_tester;
const std::vector<AutofillProfile*> profiles = {&a, &b, &c};
LogDeduplicationStartupMetrics(profiles, kLocale);
- RunUntilIdle();
// Expect two samples for `kCompany` and `kEmailAddress`, since:
// - `kCompany` and `kEmailAddress` are each emitted once by `a`.
// - `kCompany` is emitted once by `b`.
@@ -83,8 +70,8 @@
base::Bucket(SettingsVisibleFieldTypeForMetrics::kEmailAddress, 2)));
}
-TEST_F(ProfileDeduplicationMetricsTest,
- Import_RankOfStoredQuasiDuplicateProfiles) {
+TEST(ProfileDeduplicationMetricsTest,
+ Import_RankOfStoredQuasiDuplicateProfiles) {
AutofillProfile existing_profile = test::GetFullProfile();
// `import_candidate` has duplication rank 1 with `existing_profile`.
AutofillProfile import_candidate = test::GetFullProfile();
@@ -99,7 +86,7 @@
1, 1);
}
-TEST_F(ProfileDeduplicationMetricsTest, Import_TypeOfQuasiDuplicateToken) {
+TEST(ProfileDeduplicationMetricsTest, Import_TypeOfQuasiDuplicateToken) {
AutofillProfile existing_profile = test::GetFullProfile();
// `import_candidate` has duplication rank 1 with `existing_profile`.
AutofillProfile import_candidate = test::GetFullProfile();
