Optimizing OneShotTimer::reset() function
When running simpleperf, reset() function caused every once
in a while locks, which we suspect cause occasional janking.
- Moving the state to atomic, so there is no time spent
acquiring the lock.
- When using condition_variable, atomic still needs to be
changed under mutex, so moving to semaphores to guard
and signal.
See b/170665374 for analysis report.
Bug: 170665374
Test: atest DisplayMicrobenchTests while running simple perf
Test: atest OneShotTimerTest
Change-Id: Ic450ea074bc07175c3fc681b69a8009be84d30a0
diff --git a/services/surfaceflinger/Scheduler/OneShotTimer.cpp b/services/surfaceflinger/Scheduler/OneShotTimer.cpp
index a90d05e..2783800 100644
--- a/services/surfaceflinger/Scheduler/OneShotTimer.cpp
+++ b/services/surfaceflinger/Scheduler/OneShotTimer.cpp
@@ -20,6 +20,21 @@
#include <sstream>
#include <thread>
+namespace {
+using namespace std::chrono_literals;
+
+constexpr int64_t kNsToSeconds = std::chrono::duration_cast<std::chrono::nanoseconds>(1s).count();
+
+// The syscall interface uses a pair of integers for the timestamp. The first
+// (tv_sec) is the whole count of seconds. The second (tv_nsec) is the
+// nanosecond part of the count. This function takes care of translation.
+void calculateTimeoutTime(std::chrono::nanoseconds timestamp, timespec* spec) {
+ clock_gettime(CLOCK_MONOTONIC, spec);
+ spec->tv_sec += static_cast<__kernel_time_t>(timestamp.count() / kNsToSeconds);
+ spec->tv_nsec += timestamp.count() % kNsToSeconds;
+}
+} // namespace
+
namespace android {
namespace scheduler {
@@ -32,81 +47,95 @@
}
void OneShotTimer::start() {
- {
- std::lock_guard<std::mutex> lock(mMutex);
- mState = TimerState::RESET;
+ sem_init(&mSemaphore, 0, 0);
+
+ if (!mThread.joinable()) {
+ // Only create thread if it has not been created.
+ mThread = std::thread(&OneShotTimer::loop, this);
}
- mThread = std::thread(&OneShotTimer::loop, this);
}
void OneShotTimer::stop() {
- {
- std::lock_guard<std::mutex> lock(mMutex);
- mState = TimerState::STOPPED;
- }
- mCondition.notify_all();
+ mStopTriggered = true;
+ sem_post(&mSemaphore);
+
if (mThread.joinable()) {
mThread.join();
+ sem_destroy(&mSemaphore);
}
}
void OneShotTimer::loop() {
+ TimerState state = TimerState::RESET;
while (true) {
bool triggerReset = false;
bool triggerTimeout = false;
- {
- std::lock_guard<std::mutex> lock(mMutex);
- if (mState == TimerState::STOPPED) {
- break;
- }
- if (mState == TimerState::IDLE) {
- mCondition.wait(mMutex);
- continue;
- }
-
- if (mState == TimerState::RESET) {
- triggerReset = true;
- }
+ state = checkForResetAndStop(state);
+ if (state == TimerState::STOPPED) {
+ break;
}
+
+ if (state == TimerState::IDLE) {
+ sem_wait(&mSemaphore);
+ continue;
+ }
+
+ if (state == TimerState::RESET) {
+ triggerReset = true;
+ }
+
if (triggerReset && mResetCallback) {
mResetCallback();
}
- { // lock the mutex again. someone might have called stop meanwhile
- std::lock_guard<std::mutex> lock(mMutex);
- if (mState == TimerState::STOPPED) {
- break;
- }
+ state = checkForResetAndStop(state);
+ if (state == TimerState::STOPPED) {
+ break;
+ }
- auto triggerTime = std::chrono::steady_clock::now() + mInterval;
- mState = TimerState::WAITING;
- while (mState == TimerState::WAITING) {
- constexpr auto zero = std::chrono::steady_clock::duration::zero();
- auto waitTime = triggerTime - std::chrono::steady_clock::now();
- if (waitTime > zero) mCondition.wait_for(mMutex, waitTime);
- if (mState == TimerState::RESET) {
- triggerTime = std::chrono::steady_clock::now() + mInterval;
- mState = TimerState::WAITING;
- } else if (mState == TimerState::WAITING &&
- (triggerTime - std::chrono::steady_clock::now()) <= zero) {
- triggerTimeout = true;
- mState = TimerState::IDLE;
- }
+ auto triggerTime = std::chrono::steady_clock::now() + mInterval;
+ state = TimerState::WAITING;
+ while (state == TimerState::WAITING) {
+ constexpr auto zero = std::chrono::steady_clock::duration::zero();
+ // Wait for mInterval time for semaphore signal.
+ struct timespec ts;
+ calculateTimeoutTime(std::chrono::nanoseconds(mInterval), &ts);
+ sem_clockwait(&mSemaphore, CLOCK_MONOTONIC, &ts);
+
+ state = checkForResetAndStop(state);
+ if (state == TimerState::RESET) {
+ triggerTime = std::chrono::steady_clock::now() + mInterval;
+ state = TimerState::WAITING;
+ } else if (state == TimerState::WAITING &&
+ (triggerTime - std::chrono::steady_clock::now()) <= zero) {
+ triggerTimeout = true;
+ state = TimerState::IDLE;
}
}
+
if (triggerTimeout && mTimeoutCallback) {
mTimeoutCallback();
}
}
}
-void OneShotTimer::reset() {
- {
- std::lock_guard<std::mutex> lock(mMutex);
- mState = TimerState::RESET;
+OneShotTimer::TimerState OneShotTimer::checkForResetAndStop(TimerState state) {
+ // Stop takes precedence of the reset.
+ if (mStopTriggered.exchange(false)) {
+ return TimerState::STOPPED;
}
- mCondition.notify_all();
+ // If the state was stopped, the thread was joined, and we cannot reset
+ // the timer anymore.
+ if (state != TimerState::STOPPED && mResetTriggered.exchange(false)) {
+ return TimerState::RESET;
+ }
+ return state;
+}
+
+void OneShotTimer::reset() {
+ mResetTriggered = true;
+ sem_post(&mSemaphore);
}
std::string OneShotTimer::dump() const {