Bug 1368493, TimerThread::FindNextFireTimeForCurrentThread should look at timers in order, r=bkelly
authorOlli Pettay <Olli.Pettay@helsinki.fi>
Mon, 05 Jun 2017 16:49:54 +0300
changeset 410501 3efb0fe88029168573f6e0eac1595921adca32c8
parent 410500 3ccfdcb78e8c88fdc65feff3452ae9efaf4e54a0
child 410502 4ee2e37e945275cffed70a1c10e374e5bb3aea3c
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbkelly
bugs1368493
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368493, TimerThread::FindNextFireTimeForCurrentThread should look at timers in order, r=bkelly
xpcom/tests/gtest/TestTimers.cpp
xpcom/threads/TimerThread.cpp
xpcom/threads/TimerThread.h
--- a/xpcom/tests/gtest/TestTimers.cpp
+++ b/xpcom/tests/gtest/TestTimers.cpp
@@ -335,17 +335,17 @@ TEST(Timers, FindExpirationTime)
     EXPECT_EQ(t, before) << "Found time should be equal to default";
 
     t = NS_GetTimerDeadlineHintOnCurrentThread(before, 20);
     EXPECT_TRUE(t) << "We should find a time";
     EXPECT_EQ(t, before) << "Found time should be equal to default";
 
     t = NS_GetTimerDeadlineHintOnCurrentThread(middle, 0);
     EXPECT_TRUE(t) << "We should find a time";
-    EXPECT_EQ(t, middle) << "Found time should be equal to default";
+    EXPECT_LT(t, middle) << "Found time should be equal to default";
 
     t = NS_GetTimerDeadlineHintOnCurrentThread(middle, 10);
     EXPECT_TRUE(t) << "We should find a time";
     EXPECT_EQ(t, middle) << "Found time should be equal to default";
 
     t = NS_GetTimerDeadlineHintOnCurrentThread(middle, 20);
     EXPECT_TRUE(t) << "We should find a time";
     EXPECT_EQ(t, middle) << "Found time should be equal to default";
--- a/xpcom/threads/TimerThread.cpp
+++ b/xpcom/threads/TimerThread.cpp
@@ -592,52 +592,63 @@ TimerThread::RemoveTimer(nsTimerImpl* aT
 
 TimeStamp
 TimerThread::FindNextFireTimeForCurrentThread(TimeStamp aDefault, uint32_t aSearchBound)
 {
   MonitorAutoLock lock(mMonitor);
   TimeStamp timeStamp = aDefault;
   uint32_t index = 0;
 
-  for (auto timers = mTimers.begin(); timers != mTimers.end(); ++timers) {
-    nsTimerImpl* timer = (*timers)->Value();
+#ifdef DEBUG
+  TimeStamp firstTimeStamp;
+  if (!mTimers.IsEmpty()) {
+    firstTimeStamp = mTimers[0]->Timeout();
+  }
+#endif
 
-    if (!timer) {
-      continue;
-    }
+  auto end = mTimers.end();
+  while(end != mTimers.begin()) {
+    nsTimerImpl* timer = mTimers[0]->Value();
+    if (timer) {
+      if (timer->mTimeout > aDefault) {
+        timeStamp = aDefault;
+        break;
+      }
 
-    if (timer->mTimeout > aDefault) {
-      timeStamp = aDefault;
-      break;
-    }
+      // Don't yield to timers created with the *_LOW_PRIORITY type.
+      if (!timer->IsLowPriority()) {
+        bool isOnCurrentThread = false;
+        nsresult rv = timer->mEventTarget->IsOnCurrentThread(&isOnCurrentThread);
+        if (NS_SUCCEEDED(rv) && isOnCurrentThread) {
+          timeStamp = timer->mTimeout;
+          break;
+        }
+      }
 
-    // Don't yield to timers created with the *_LOW_PRIORITY type.
-    if (timer->IsLowPriority()) {
-      continue;
+      if (++index > aSearchBound) {
+        // Track the currently highest timeout so that we can bail out when we
+        // reach the bound or when we find a timer for the current thread.
+        // This won't give accurate information if we stop before finding
+        // any timer for the current thread, but at least won't report too
+        // long idle period.
+        timeStamp = timer->mTimeout;
+        break;
+      }
     }
 
-    // Track the currently highest timeout so that we can bail when we
-    // reach the bound or when we find a timer for the current thread.
-    timeStamp = timer->mTimeout;
-
-    bool isOnCurrentThread = false;
-    nsresult rv = timer->mEventTarget->IsOnCurrentThread(&isOnCurrentThread);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      continue;
-    }
+    std::pop_heap(mTimers.begin(), end, Entry::UniquePtrLessThan);
+    --end;
+  }
 
-    if (isOnCurrentThread) {
-
-      break;
-    }
+  while (end != mTimers.end()) {
+    ++end;
+    std::push_heap(mTimers.begin(), end, Entry::UniquePtrLessThan);
+  }
 
-    if (++index > aSearchBound) {
-      break;
-    }
-  }
+  MOZ_ASSERT_IF(!mTimers.IsEmpty(), firstTimeStamp == mTimers[0]->Timeout());
 
   return timeStamp;
 }
 
 // This function must be called from within a lock
 bool
 TimerThread::AddTimerInternal(nsTimerImpl* aTimer)
 {
--- a/xpcom/threads/TimerThread.h
+++ b/xpcom/threads/TimerThread.h
@@ -111,16 +111,21 @@ private:
 
     static bool
     UniquePtrLessThan(UniquePtr<Entry>& aLeft, UniquePtr<Entry>& aRight)
     {
       // This is reversed because std::push_heap() sorts the "largest" to
       // the front of the heap.  We want that to be the earliest timer.
       return aRight->mTimeout < aLeft->mTimeout;
     }
+
+    TimeStamp Timeout() const
+    {
+      return mTimeout;
+    }
   };
 
   nsTArray<UniquePtr<Entry>> mTimers;
   uint32_t mAllowedEarlyFiringMicroseconds;
 };
 
 struct TimerAdditionComparator
 {