Bug 1961386 - part 6: Use binary search in Add/RemoveTimerInternal. r=xpcom-reviewers,nika,jlink

The linear scan for the insertion point can be replaced by a binary search.

All the other logic of finding empty slots or removing entries remains unchanged by this.

Resulting complexity:
- Changed: AddTimer becomes minimum O(log[n]) (due to the lower boundary of the binary search) to maximum O(n) depending on the luck we have with finding empty slots.
- Changed: RemoveTimer becomes O(log(n)) always (for the binary search) as we postpone the removal of canceled timers to happen in the Run loop or through re-use.
- Extracting the next timer to fire on the TimerThread remains always O(n) (but happens on the timer thread, where it might disturb less).
- Removing the leading canceled timers remains always O(n) (but happens on the timer thread, where it might disturb less).

Differential Revision: https://phabricator.services.mozilla.com/D249902

xpcom/threads/TimerThread.cpp

@@ -670,21 +670,6 @@ struct IntervalComparator {
 
 }  // namespace
 
-size_t TimerThread::ComputeTimerInsertionIndex(const TimeStamp& timeout) const {
-  mMonitor.AssertCurrentThreadOwns();
-
-  const size_t timerCount = mTimers.Length();
-
-  size_t firstGtIndex = 0;
-  while (firstGtIndex < timerCount &&
-         (!mTimers[firstGtIndex].mTimerImpl ||
-          mTimers[firstGtIndex].mTimeout <= timeout)) {
-    ++firstGtIndex;
-  }
-
-  return firstGtIndex;
-}
-
 TimeStamp TimerThread::ComputeWakeupTimeFromTimers() const {
   mMonitor.AssertCurrentThreadOwns();
 
@@ -1126,6 +1111,14 @@ TimeStamp TimerThread::FindNextFireTimeForCurrentThread(TimeStamp aDefault,
   return aDefault;
 }
 
+void TimerThread::AssertTimersSortedAndUnique() {
+  MOZ_ASSERT(std::is_sorted(mTimers.begin(), mTimers.end()),
+             "mTimers must be sorted.");
+  MOZ_ASSERT(
+      std::adjacent_find(mTimers.begin(), mTimers.end()) == mTimers.end(),
+      "mTimers must not contain duplicate entries.");
+}
+
 // This function must be called from within a lock
 // Also: we hold the mutex for the nsTimerImpl.
 void TimerThread::AddTimerInternal(nsTimerImpl& aTimer) {
@@ -1134,11 +1127,9 @@ void TimerThread::AddTimerInternal(nsTimerImpl& aTimer) {
   AUTO_TIMERS_STATS(TimerThread_AddTimerInternal);
   LogTimerEvent::LogDispatch(&aTimer);
 
-  // TODO: Add is_sorted check after changing our book-keeping.
-
   // Do the AddRef here.
   Entry toBeAdded{aTimer};
-  size_t insertAt = ComputeTimerInsertionIndex(aTimer.mTimeout);
+  size_t insertAt = mTimers.IndexOfFirstElementGt(toBeAdded);
 
   if (insertAt > 0 && !mTimers[insertAt - 1].mTimerImpl) {
     // Very common scenario in practice: The timer just before the insertion
@@ -1148,6 +1139,7 @@ void TimerThread::AddTimerInternal(nsTimerImpl& aTimer) {
     // our very own canceled slot here, given the order of the array.
     AUTO_TIMERS_STATS(TimerThread_AddTimerInternal_ReuseBefore);
     mTimers[insertAt - 1] = std::move(toBeAdded);
+    AssertTimersSortedAndUnique();
     return;
   }
 
@@ -1173,6 +1165,8 @@ void TimerThread::AddTimerInternal(nsTimerImpl& aTimer) {
     AUTO_TIMERS_STATS(TimerThread_AddTimerInternal_Expand);
     mTimers.AppendElement(std::move(toBeAdded));
   }
+
+  AssertTimersSortedAndUnique();
 }
 
 // This function must be called from within a lock
@@ -1186,15 +1180,15 @@ bool TimerThread::RemoveTimerInternal(nsTimerImpl& aTimer) {
     return false;
   }
 
-  // TODO: Add is_sorted check after changing our book-keeping.
-
-  AUTO_TIMERS_STATS(TimerThread_RemoveTimerInternal_in_list);
-  for (auto& entry : mTimers) {
-    if (entry.mTimerImpl == &aTimer) {
-      entry.mTimerImpl = nullptr;
+  size_t removeAt = mTimers.BinaryIndexOf(EntryKey{aTimer});
+  if (removeAt != nsTArray<Entry>::NoIndex) {
+    MOZ_ASSERT(mTimers[removeAt].mTimerImpl == &aTimer);
+    // Mark the timer as canceled, defer the removal to the timer thread.
+    mTimers[removeAt].mTimerImpl = nullptr;
+    AssertTimersSortedAndUnique();
     return true;
   }
-  }
+
   MOZ_ASSERT_UNREACHABLE("Not found in the list but it should be!?");
   return false;
 }
@@ -1203,6 +1197,9 @@ void TimerThread::RemoveLeadingCanceledTimersInternal() {
   mMonitor.AssertCurrentThreadOwns();
   AUTO_TIMERS_STATS(TimerThread_RemoveLeadingCanceledTimersInternal);
 
+  // Let's check if we are still sorted before removing the canceled timers.
+  AssertTimersSortedAndUnique();
+
   size_t toRemove = 0;
   while (toRemove < mTimers.Length() && !mTimers[toRemove].mTimerImpl) {
     ++toRemove;

xpcom/threads/TimerThread.h

@@ -70,6 +70,7 @@ class TimerThread final : public mozilla::Runnable, public nsIObserver {
       MOZ_REQUIRES(mMonitor, aTimer.mMutex);
   void RemoveLeadingCanceledTimersInternal() MOZ_REQUIRES(mMonitor);
   nsresult Init() MOZ_REQUIRES(mMonitor);
+  void AssertTimersSortedAndUnique() MOZ_REQUIRES(mMonitor);
 
   // Using atomic because this value is written to in one place, and read from
   // in another, and those two locations are likely to be executed from separate
@@ -91,11 +92,37 @@ class TimerThread final : public mozilla::Runnable, public nsIObserver {
   bool mNotified MOZ_GUARDED_BY(mMonitor);
   bool mSleeping MOZ_GUARDED_BY(mMonitor);
 
-  struct Entry final {
+  struct EntryKey {
+    explicit EntryKey(nsTimerImpl& aTimerImpl)
+        : mTimeout(aTimerImpl.mTimeout), mTimerSeq(aTimerImpl.mTimerSeq) {}
+
+    // The comparison operators must ensure to detect equality only for
+    // equal mTimerImpl except for canceled timers.
+    // This is achieved through the sequence number.
+    // Currently we maintain a FIFO order for timers with equal timeout.
+    // Note that it might make sense to flip the sequence order to favor
+    // timeouts with smaller delay as they are most likely more sensitive
+    // to jitter. But we strictly test for FIFO order in our gtests.
+
+    bool operator==(const EntryKey& aRhs) const {
+      return (mTimeout == aRhs.mTimeout && mTimerSeq == aRhs.mTimerSeq);
+    }
+
+    bool operator<(const EntryKey& aRhs) const {
+      if (mTimeout == aRhs.mTimeout) {
+        return mTimerSeq < aRhs.mTimerSeq;
+      }
+      return mTimeout < aRhs.mTimeout;
+    }
+
+    TimeStamp mTimeout;
+    uint64_t mTimerSeq;
+  };
+
+  struct Entry final : EntryKey {
     explicit Entry(nsTimerImpl& aTimerImpl)
-        : mTimeout(aTimerImpl.mTimeout),
+        : EntryKey(aTimerImpl),
           mDelay(aTimerImpl.mDelay),
-          mTimerSeq(aTimerImpl.mTimerSeq),
           mTimerImpl(&aTimerImpl) {}
 
     // No copies to not fiddle with mTimerImpl's ref-count.
@@ -114,23 +141,12 @@ class TimerThread final : public mozilla::Runnable, public nsIObserver {
     }
 #endif
 
-    // These values are simply cached from the timer. Keeping them here is good
-    // for cache usage and allows us to avoid worrying about locking conflicts
-    // with the timer.
-    TimeStamp mTimeout;
     TimeDuration mDelay;
-    uint64_t mTimerSeq;
-
     RefPtr<nsTimerImpl> mTimerImpl;
   };
 
   void PostTimerEvent(Entry& aPostMe) MOZ_REQUIRES(mMonitor);
 
-  // Computes and returns the index in mTimers at which a new timer with the
-  // specified timeout should be inserted in order to maintain "sorted" order.
-  size_t ComputeTimerInsertionIndex(const TimeStamp& timeout) const
-      MOZ_REQUIRES(mMonitor);
-
   // Computes and returns when we should next try to wake up in order to handle
   // the triggering of the timers in mTimers.
   // If mTimers is empty, returns a null TimeStamp. If mTimers is not empty,
@@ -160,10 +176,9 @@ class TimerThread final : public mozilla::Runnable, public nsIObserver {
   // clears a few flags before and after.
   void Wait(TimeDuration aWaitFor) MOZ_REQUIRES(mMonitor);
 
-  // mTimers is maintained in a "pseudo-sorted" order wrt the timeouts.
-  // Specifcally, mTimers is sorted according to the timeouts *if you ignore the
-  // canceled entries* (those whose mTimerImpl is nullptr). Notably this means
-  // that you cannot use a binary search on this list.
+  // mTimers is sorted by timeout, followed by a unique sequence number.
+  // Some entries are for cancelled entries, but remain in sorted order based
+  // on the timeout and sequence number they were originally created with.
   nsTArray<Entry> mTimers MOZ_GUARDED_BY(mMonitor);
 
   // Set only at the start of the thread's Run():