commit aadcf949e345fd1fc818e37265bda0c29fcf5d8b
author Alec Mouri <alecmouri@google.com> Wed May 11 01:22:42 2022 +0000
committer Alec Mouri <alecmouri@google.com> Wed May 18 22:24:10 2022 +0000
tree 638d44f3c306beaa282acd94e1d1aa0d2eda5e6d
parent 8ffb0288806aa7f41ff1b6f6ad9321980f955907

Reduce gratuitous resets for OneShotTimer

OneShotTimer::reset just resets an internal timer thread for it to go
back to sleep. When just implemented with semaphores, this introduces a
lot of syscall churn due to posting a semaphore -> waking up the timer
thread -> having the thread go back to sleep. Instead, we can avoid
resetting the timer if we know that the thread is about to wait for a
short amount of time, and only reset the timer if the thread is in an
idle state.

This patch reduces instruction counts by 5%, and CPU cycles incurred by
an additional 5%.

Bug: 232272570
Test: bouncy ball

Change-Id: I83f968042395857237875aab8dca0e6b90d392cb

services/surfaceflinger/Scheduler/OneShotTimer.cpp

diff --git a/services/surfaceflinger/Scheduler/OneShotTimer.cpp b/services/surfaceflinger/Scheduler/OneShotTimer.cpp
index 16f041a..9c6e56d 100644
--- a/services/surfaceflinger/Scheduler/OneShotTimer.cpp
+++ b/services/surfaceflinger/Scheduler/OneShotTimer.cpp
@@ -47,6 +47,7 @@
         mInterval(interval),
         mResetCallback(resetCallback),
         mTimeoutCallback(timeoutCallback) {
+    mLastResetTime = std::chrono::steady_clock::time_point::min();
     LOG_ALWAYS_FATAL_IF(!mClock, "Clock must not be provided");
 }
 
@@ -116,9 +117,10 @@
 
         auto triggerTime = mClock->now() + mInterval;
         state = TimerState::WAITING;
-        while (state == TimerState::WAITING) {
+        while (true) {
+            mWaiting = true;
             constexpr auto zero = std::chrono::steady_clock::duration::zero();
-            // Wait for mInterval time for semaphore signal.
+            // Wait for mInterval time to check if we need to reset or drop into the idle state.
             struct timespec ts;
             calculateTimeoutTime(std::chrono::nanoseconds(mInterval), &ts);
             int result = sem_clockwait(&mSemaphore, CLOCK_MONOTONIC, &ts);
@@ -128,13 +130,21 @@
                 LOG_ALWAYS_FATAL("%s", ss.str().c_str());
             }
 
+            mWaiting = false;
             state = checkForResetAndStop(state);
-            if (state == TimerState::RESET) {
-                triggerTime = mClock->now() + mInterval;
-                state = TimerState::WAITING;
-            } else if (state == TimerState::WAITING && (triggerTime - mClock->now()) <= zero) {
+            if (state == TimerState::STOPPED) {
+                break;
+            }
+
+            if (state == TimerState::WAITING && (triggerTime - mClock->now()) <= zero) {
                 triggerTimeout = true;
                 state = TimerState::IDLE;
+                break;
+            }
+
+            if (state == TimerState::RESET) {
+                triggerTime = mLastResetTime.load() + mInterval;
+                state = TimerState::WAITING;
             }
         }
 
@@ -158,9 +168,14 @@
 }
 
 void OneShotTimer::reset() {
+    mLastResetTime = mClock->now();
     mResetTriggered = true;
-    int result = sem_post(&mSemaphore);
-    LOG_ALWAYS_FATAL_IF(result, "sem_post failed");
+    // If mWaiting is true, then we are guaranteed to be in a block where we are waiting on
+    // mSemaphore for a timeout, rather than idling. So we can avoid a sem_post call since we can
+    // just check that we triggered a reset on timeout.
+    if (!mWaiting) {
+        LOG_ALWAYS_FATAL_IF(sem_post(&mSemaphore), "sem_post failed");
+    }
 }
 
 std::string OneShotTimer::dump() const {