Reduce the number of tasks posted by FieldTrialSynchronizer

During startup lots of tiny tasks are posted by FieldTrialSynchronizer.
Individually they are not a problem but on low end devices it can take
60+ ms to process all of these. This patch significantly reduces the
number of tasks posted.

Change-Id: I1d0bff3386cfb308c27641f90189aae2071a5fe0
BUG: 1013535
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849852Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Auto-Submit: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720908}

base/metrics/field_trial.cc

@@ -22,6 +22,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/trace_event/trace_event.h"
 #include "base/unguessable_token.h"
 
 // On POSIX, the fd is shared using the mapping in GlobalDescriptors.
@@ -971,6 +972,7 @@ void FieldTrialList::OnGroupFinalized(bool is_locked, FieldTrial* field_trial) {
 
 // static
 void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
+  TRACE_EVENT0("base", "FieldTrialList::NotifyFieldTrialGroupSelection");
   if (!global_)
     return;
 
@@ -999,7 +1001,7 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
         field_trial->trial_name(), field_trial->group_name_internal());
   }
 
-  global_->observer_list_->Notify(
+  global_->observer_list_->NotifySynchronously(
       FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
       field_trial->trial_name(), field_trial->group_name_internal());
 }

base/metrics/field_trial_unittest.cc

@@ -948,11 +948,6 @@ TEST_F(FieldTrialTest, Observe) {
   const int chosen_group = trial->group();
   EXPECT_TRUE(chosen_group == default_group || chosen_group == secondary_group);
 
-  // Observers are called asynchronously.
-  EXPECT_TRUE(observer.trial_name().empty());
-  EXPECT_TRUE(observer.group_name().empty());
-  RunLoop().RunUntilIdle();
-
   EXPECT_EQ(kTrialName, observer.trial_name());
   if (chosen_group == default_group)
     EXPECT_EQ(kDefaultGroupName, observer.group_name());

base/observer_list_threadsafe.h

@@ -168,6 +168,41 @@ class ObserverListThreadSafe : public internal::ObserverListThreadSafeBase {
     }
   }
 
+  // Like Notify() but attempts to synchronously invoke callbacks if they are
+  // associated with this thread.
+  template <typename Method, typename... Params>
+  void NotifySynchronously(const Location& from_here,
+                           Method m,
+                           Params&&... params) {
+    RepeatingCallback<void(ObserverType*)> method =
+        BindRepeating(&Dispatcher<ObserverType, Method>::Run, m,
+                      std::forward<Params>(params)...);
+
+    // The observers may make reentrant calls (which can be a problem due to the
+    // lock), so we extract a list to call synchronously.
+    std::vector<ObserverType*> current_sequence_observers;
+
+    {
+      AutoLock lock(lock_);
+      current_sequence_observers.reserve(observers_.size());
+      for (const auto& observer : observers_) {
+        if (observer.second->RunsTasksInCurrentSequence()) {
+          current_sequence_observers.push_back(observer.first);
+        } else {
+          observer.second->PostTask(
+              from_here,
+              BindOnce(&ObserverListThreadSafe<ObserverType>::NotifyWrapper,
+                       this, observer.first,
+                       NotificationData(this, from_here, method)));
+        }
+      }
+    }
+
+    for (ObserverType* observer : current_sequence_observers) {
+      NotifyWrapper(observer, NotificationData(this, from_here, method));
+    }
+  }
+
  private:
   friend class RefCountedThreadSafe<ObserverListThreadSafeBase>;
 

base/observer_list_threadsafe_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool/thread_pool_instance.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_restrictions.h"
@@ -502,4 +503,58 @@ TEST(ObserverListThreadSafeTest, Existing) {
   EXPECT_EQ(1, c.total);
 }
 
+TEST(ObserverListThreadSafeTest, NotifySynchronously) {
+  test::TaskEnvironment task_environment;
+
+  scoped_refptr<ObserverListThreadSafe<Foo>> observer_list(
+      new ObserverListThreadSafe<Foo>);
+  Adder a(1);
+  Adder b(-1);
+  Adder c(1);
+  Adder d(-1);
+
+  observer_list->AddObserver(&a);
+  observer_list->AddObserver(&b);
+
+  observer_list->NotifySynchronously(FROM_HERE, &Foo::Observe, 10);
+
+  observer_list->AddObserver(&c);
+  observer_list->AddObserver(&d);
+
+  observer_list->NotifySynchronously(FROM_HERE, &Foo::Observe, 10);
+
+  EXPECT_EQ(20, a.total);
+  EXPECT_EQ(-20, b.total);
+  EXPECT_EQ(10, c.total);
+  EXPECT_EQ(-10, d.total);
+}
+
+TEST(ObserverListThreadSafeTest, NotifySynchronouslyCrossSequence) {
+  test::TaskEnvironment task_environment;
+
+  scoped_refptr<ObserverListThreadSafe<Foo>> observer_list(
+      new ObserverListThreadSafe<Foo>);
+  Adder a(1);
+  observer_list->AddObserver(&a);
+
+  WaitableEvent event(WaitableEvent::ResetPolicy::AUTOMATIC,
+                      WaitableEvent::InitialState::NOT_SIGNALED);
+  // Call NotifySynchronously on a different sequence.
+  PostTask(FROM_HERE, {ThreadPool()}, BindLambdaForTesting([&]() {
+             observer_list->NotifySynchronously(FROM_HERE, &Foo::Observe, 10);
+             event.Signal();
+           }));
+
+  event.Wait();
+
+  // Because it was run on a different sequence NotifySynchronously should have
+  // posted a task which hasn't run yet.
+  EXPECT_EQ(0, a.total);
+
+  RunLoop().RunUntilIdle();
+
+  // Verify the task has now run.
+  EXPECT_EQ(10, a.total);
+}
+
 }  // namespace base

chrome/browser/metrics/field_trial_synchronizer.cc

@@ -72,9 +72,10 @@ void FieldTrialSynchronizer::OnFieldTrialGroupFinalized(
     return;
   }
 
-  base::PostTask(FROM_HERE, {BrowserThread::UI},
-                 base::BindOnce(&FieldTrialSynchronizer::NotifyAllRenderers,
-                                this, field_trial_name, group_name));
+  RunOrPostTaskOnThread(
+      FROM_HERE, BrowserThread::UI,
+      base::BindOnce(&FieldTrialSynchronizer::NotifyAllRenderers, this,
+                     field_trial_name, group_name));
 }
 
 FieldTrialSynchronizer::~FieldTrialSynchronizer() {

components/variations/variations_http_header_provider_unittest.cc

@@ -41,13 +41,15 @@ scoped_refptr<base::FieldTrial> CreateTrialAndAssociateId(
     const std::string& default_group_name,
     IDCollectionKey key,
     VariationID id) {
+  AssociateGoogleVariationID(key, trial_name, default_group_name, id);
   scoped_refptr<base::FieldTrial> trial(
       base::FieldTrialList::CreateFieldTrial(trial_name, default_group_name));
   EXPECT_TRUE(trial);
 
   if (trial) {
-    AssociateGoogleVariationID(key, trial->trial_name(), trial->group_name(),
-                               id);
+    // Ensure the trial is registered under the correct key so we can look it
+    // up.
+    trial->group();
   }
 
   return trial;