Make ObserverList iteration 10~20 times faster.

ObserverList iterators take a WeakRef to the ObserverList to detect
the list being destroyed mid-iteration. ObserverList iteration doesn't
need the thread-safety guarantees from base::WeakPtr, and there's rarely
more than one iterator, or a couple on the stack.

ObserverList iteration was identified as a performance concern as part
of https://crbug.com/859155.

A base::LinkedList can cater for detecting a destroy-while-iterating.
This avoids the malloc() performed when the WeakRef is first dereferenced
each iteration, which currently dominates iteration cost. We also
reduce reference indirection, and reduce pipelining damage of atomics.

Sample performance, for the given ObserverList size in Release. Values
are nanoseconds to notify a single observer. Old -> New. (Old values
improve with ObserverList size due to the malloc() cost being amortized
over the list size).

ObserverList::Unchecked
0:   22.3 -> 1.0
1:   79.6 -> 3.4
2:   61.6 -> 3.2
4:   50.5 -> 3.1
8:   41.0 -> 3.2
16:  33.6 -> 3.2
32:  30.3 -> 3.3
64:  28.8 -> 3.2
128: 28.0 -> 3.2

ObserverList<CheckedObserver>
0:   17.9 ->  0.8
1:   89.3 -> 12.9
2:   73.1 -> 14.8
4:   62.8 -> 16.0
8:   55.4 -> 18.0
16:  51.8 -> 18.3
32:  49.0 -> 18.3
64:  46.6 -> 18.3
128: 45.6 -> 18.2

Bug: 888973
Change-Id: I5edeb754e8c62d64efbbdecf617d5b33b0933688
Reviewed-on: https://chromium-review.googlesource.com/c/1242568
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596492}

base/BUILD.gn

@@ -2053,6 +2053,7 @@ test("base_perftests") {
     "message_loop/message_loop_perftest.cc",
     "message_loop/message_loop_task_runner_perftest.cc",
     "message_loop/message_pump_perftest.cc",
+    "observer_list_perftest.cc",
     "task/sequence_manager/sequence_manager_perftest.cc",
 
     # "test/run_all_unittests.cc",

base/observer_list.h

@@ -16,8 +16,8 @@
 #include "base/gtest_prod_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/observer_list_internal.h"
+#include "base/sequence_checker.h"
 #include "base/stl_util.h"
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -97,10 +97,7 @@ template <class ObserverType,
           bool check_empty = false,
           bool allow_reentrancy = true,
           class ObserverStorageType = internal::CheckedObserverAdapter>
-class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
-                                                         check_empty,
-                                                         allow_reentrancy,
-                                                         ObserverStorageType>> {
+class ObserverList {
  public:
   // Allow declaring an ObserverList<...>::Unchecked that replaces the default
   // ObserverStorageType to use raw pointers. This is required to support legacy
@@ -126,39 +123,42 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
     Iter() : index_(0), max_index_(0) {}
 
     explicit Iter(const ObserverList* list)
-        : list_(const_cast<ObserverList*>(list)->AsWeakPtr()),
+        : list_(const_cast<ObserverList*>(list)),
           index_(0),
           max_index_(list->policy_ == ObserverListPolicy::ALL
                          ? std::numeric_limits<size_t>::max()
                          : list->observers_.size()) {
-      DCHECK(list_);
-      DCHECK(allow_reentrancy || !list_->live_iterator_count_);
+      DCHECK(list);
+      DCHECK(allow_reentrancy || list_.IsOnlyRemainingNode());
+      // Bind to this sequence when creating the first iterator.
+      DCHECK_CALLED_ON_VALID_SEQUENCE(list_->iteration_sequence_checker_);
       EnsureValidIndex();
-      ++list_->live_iterator_count_;
     }
 
     ~Iter() {
-      if (!list_)
-        return;
-
-      DCHECK_GT(list_->live_iterator_count_, 0);
-      if (--list_->live_iterator_count_ == 0)
+      if (list_.IsOnlyRemainingNode())
         list_->Compact();
     }
 
     Iter(const Iter& other)
-        : list_(other.list_),
-          index_(other.index_),
-          max_index_(other.max_index_) {
-      if (list_)
-        ++list_->live_iterator_count_;
+        : index_(other.index_), max_index_(other.max_index_) {
+      if (other.list_)
+        list_.SetList(other.list_.get());
     }
 
-    Iter& operator=(Iter other) {
-      using std::swap;
-      swap(list_, other.list_);
-      swap(index_, other.index_);
-      swap(max_index_, other.max_index_);
+    Iter& operator=(const Iter& other) {
+      if (&other == this)
+        return *this;
+
+      if (list_.IsOnlyRemainingNode())
+        list_->Compact();
+
+      list_.Invalidate();
+      if (other.list_)
+        list_.SetList(other.list_.get());
+
+      index_ = other.index_;
+      max_index_ = other.max_index_;
       return *this;
     }
 
@@ -220,7 +220,8 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
 
     bool is_end() const { return !list_ || index_ == clamped_max_index(); }
 
-    WeakPtr<ObserverList> list_;
+    // Lightweight weak pointer to the ObserverList.
+    internal::WeakLinkNode<ObserverList> list_;
 
     // When initially constructed and each time the iterator is incremented,
     // |index_| is guaranteed to point to a non-null index if the iterator
@@ -240,10 +241,19 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
 
   const_iterator end() const { return const_iterator(); }
 
-  ObserverList() = default;
+  ObserverList() {
+    // Sequence checks only apply when iterators are live.
+    DETACH_FROM_SEQUENCE(iteration_sequence_checker_);
+  }
   explicit ObserverList(ObserverListPolicy policy) : policy_(policy) {}
 
   ~ObserverList() {
+    // If there are live iterators, ensure destruction is thread-safe.
+    if (!live_iterators_.empty())
+      DCHECK_CALLED_ON_VALID_SEQUENCE(iteration_sequence_checker_);
+
+    while (!live_iterators_.empty())
+      live_iterators_.head()->value()->Invalidate();
     if (check_empty) {
       Compact();
       DCHECK(observers_.empty());
@@ -274,11 +284,11 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
     if (it == observers_.end())
       return;
 
-    DCHECK_GE(live_iterator_count_, 0);
-    if (live_iterator_count_) {
-      it->MarkForRemoval();
-    } else {
+    if (live_iterators_.empty()) {
       observers_.erase(it);
+    } else {
+      DCHECK_CALLED_ON_VALID_SEQUENCE(iteration_sequence_checker_);
+      it->MarkForRemoval();
     }
   }
 
@@ -296,33 +306,37 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
 
   // Removes all the observers from this list.
   void Clear() {
-    DCHECK_GE(live_iterator_count_, 0);
-    if (live_iterator_count_) {
+    if (live_iterators_.empty()) {
+      observers_.clear();
+    } else {
+      DCHECK_CALLED_ON_VALID_SEQUENCE(iteration_sequence_checker_);
       for (auto& observer : observers_)
         observer.MarkForRemoval();
-    } else {
-      observers_.clear();
     }
   }
 
   bool might_have_observers() const { return !observers_.empty(); }
 
  private:
+  friend class internal::WeakLinkNode<ObserverList>;
+
   // Compacts list of observers by removing those marked for removal.
   void Compact() {
+    // Detach whenever the last iterator is destroyed. Detaching is safe because
+    // Compact() is only ever called when the last iterator is destroyed.
+    DETACH_FROM_SEQUENCE(iteration_sequence_checker_);
+
     EraseIf(observers_, [](const auto& o) { return o.IsMarkedForRemoval(); });
   }
 
   std::vector<ObserverStorageType> observers_;
 
-  // Number of active iterators referencing this ObserverList.
-  //
-  // This counter is not synchronized although it is modified by const
-  // iterators.
-  int live_iterator_count_ = 0;
+  base::LinkedList<internal::WeakLinkNode<ObserverList>> live_iterators_;
 
   const ObserverListPolicy policy_ = ObserverListPolicy::ALL;
 
+  SEQUENCE_CHECKER(iteration_sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(ObserverList);
 };
 

base/observer_list_internal.h

@@ -6,6 +6,7 @@
 #define BASE_OBSERVER_LIST_INTERNAL_H_
 
 #include "base/base_export.h"
+#include "base/containers/linked_list.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -95,6 +96,52 @@ class BASE_EXPORT CheckedObserverAdapter {
   DISALLOW_COPY_AND_ASSIGN(CheckedObserverAdapter);
 };
 
+// Wraps a pointer in a stack-allocated, base::LinkNode. The node is
+// automatically removed from the linked list upon destruction (of the node, not
+// the pointer). Nodes are detached from the list via Invalidate() in the
+// destructor of ObserverList. This invalidates all WeakLinkNodes. There is no
+// threading support.
+template <class ObserverList>
+class WeakLinkNode : public base::LinkNode<WeakLinkNode<ObserverList>> {
+ public:
+  WeakLinkNode() = default;
+  explicit WeakLinkNode(ObserverList* list) { SetList(list); }
+
+  ~WeakLinkNode() { Invalidate(); }
+
+  bool IsOnlyRemainingNode() const {
+    return list_ &&
+           list_->live_iterators_.head() == list_->live_iterators_.tail();
+  }
+
+  void SetList(ObserverList* list) {
+    DCHECK(!list_);
+    DCHECK(list);
+    list_ = list;
+    list_->live_iterators_.Append(this);
+  }
+
+  void Invalidate() {
+    if (list_) {
+      list_ = nullptr;
+      this->RemoveFromList();
+    }
+  }
+
+  ObserverList* get() const {
+    if (list_)
+      DCHECK_CALLED_ON_VALID_SEQUENCE(list_->iteration_sequence_checker_);
+    return list_;
+  }
+  ObserverList* operator->() const { return get(); }
+  explicit operator bool() const { return get(); }
+
+ private:
+  ObserverList* list_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(WeakLinkNode);
+};
+
 }  // namespace internal
 }  // namespace base
 

base/observer_list_perftest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/observer_list.h"
+
+#include <memory>
+
+#include "base/logging.h"
+#include "base/observer_list.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/perf/perf_test.h"
+
+// Ask the compiler not to use a register for this counter, in case it decides
+// to do magic optimizations like |counter += kLaps|.
+volatile int g_observer_list_perf_test_counter;
+
+namespace base {
+
+class ObserverInterface {
+ public:
+  ObserverInterface() {}
+  virtual ~ObserverInterface() {}
+  virtual void Observe() const { ++g_observer_list_perf_test_counter; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ObserverInterface);
+};
+
+class UnsafeObserver : public ObserverInterface {};
+
+class TestCheckedObserver : public CheckedObserver, public ObserverInterface {};
+
+template <class ObserverType>
+struct Pick {
+  // The ObserverList type to use. Checked observers need to be in a checked
+  // ObserverList.
+  using ObserverListType = ObserverList<ObserverType>;
+  static const char* GetName() { return "CheckedObserver"; }
+};
+template <>
+struct Pick<UnsafeObserver> {
+  using ObserverListType = ObserverList<ObserverInterface>::Unchecked;
+  static const char* GetName() { return "UnsafeObserver"; }
+};
+
+template <class ObserverType>
+class ObserverListPerfTest : public ::testing::Test {
+ public:
+  using ObserverListType = typename Pick<ObserverType>::ObserverListType;
+
+  ObserverListPerfTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ObserverListPerfTest);
+};
+
+typedef ::testing::Types<UnsafeObserver, TestCheckedObserver> ObserverTypes;
+TYPED_TEST_CASE(ObserverListPerfTest, ObserverTypes);
+
+// Performance test for base::ObserverList and Checked Observers.
+TYPED_TEST(ObserverListPerfTest, NotifyPerformance) {
+  constexpr int kMaxObservers = 128;
+#if DCHECK_IS_ON()
+  // The test takes about 100x longer in debug builds, mostly due to sequence
+  // checker overheads when WeakPtr gets involved.
+  constexpr int kLaps = 1000000;
+#else
+  constexpr int kLaps = 100000000;
+#endif
+  constexpr int kWarmupLaps = 100;
+  std::vector<std::unique_ptr<TypeParam>> observers;
+
+  for (int observer_count = 0; observer_count <= kMaxObservers;
+       observer_count = observer_count ? observer_count * 2 : 1) {
+    typename TestFixture::ObserverListType list;
+    for (int i = 0; i < observer_count; ++i)
+      observers.push_back(std::make_unique<TypeParam>());
+    for (auto& o : observers)
+      list.AddObserver(o.get());
+
+    for (int i = 0; i < kWarmupLaps; ++i) {
+      for (auto& o : list)
+        o.Observe();
+    }
+    g_observer_list_perf_test_counter = 0;
+    const int weighted_laps = kLaps / (observer_count + 1);
+
+    TimeTicks start = TimeTicks::Now();
+    for (int i = 0; i < weighted_laps; ++i) {
+      for (auto& o : list)
+        o.Observe();
+    }
+    TimeDelta duration = TimeTicks::Now() - start;
+
+    const char* name = Pick<TypeParam>::GetName();
+    observers.clear();
+
+    EXPECT_EQ(observer_count * weighted_laps,
+              g_observer_list_perf_test_counter);
+    EXPECT_TRUE(observer_count == 0 || list.might_have_observers());
+
+    std::string prefix =
+        base::StringPrintf("ObserverListPerfTest_%d.", observer_count);
+
+    // A typical value is 3-20 nanoseconds per observe in Release, 1000-2000ns
+    // in an optimized build with DCHECKs and 3000-6000ns in debug builds.
+    perf_test::PrintResult(
+        prefix, name, "NotifyPerformance",
+        duration.InNanoseconds() /
+            static_cast<double>(g_observer_list_perf_test_counter +
+                                weighted_laps),
+        "ns/observe", true);
+  }
+}
+
+}  // namespace base

base/observer_list_unittest.cc

@@ -120,8 +120,8 @@ class ObserverListTestBase {
   ObserverListTestBase() {}
 
   template <class T>
-  const decltype(T::list_) list(const T& iter) {
-    return iter.list_;
+  const decltype(T::list_.get()) list(const T& iter) {
+    return iter.list_.get();
   }
 
   template <class T>