Merge "SF: Fix many flakes in IdleTimerTest"
diff --git a/services/surfaceflinger/Scheduler/IdleTimer.cpp b/services/surfaceflinger/Scheduler/IdleTimer.cpp
index 5a76dbc..e975e65 100644
--- a/services/surfaceflinger/Scheduler/IdleTimer.cpp
+++ b/services/surfaceflinger/Scheduler/IdleTimer.cpp
@@ -54,18 +54,24 @@
if (mState == TimerState::IDLE) {
mCondition.wait(mMutex);
} else if (mState == TimerState::RESET) {
+ auto triggerTime = std::chrono::steady_clock::now() + mInterval;
mState = TimerState::WAITING;
- if (mCondition.wait_for(mMutex, mInterval) == std::cv_status::timeout) {
- if (mTimeoutCallback) {
- mTimeoutCallback();
+ 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::WAITING &&
+ (triggerTime - std::chrono::steady_clock::now()) <= zero) {
+ if (mTimeoutCallback) {
+ mTimeoutCallback();
+ }
+
+ mState = TimerState::IDLE;
}
}
- if (mState == TimerState::WAITING) {
- mState = TimerState::IDLE;
- }
}
}
-}
+} // namespace scheduler
void IdleTimer::reset() {
{
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 00820f1..6473afd 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -81,7 +81,10 @@
}
}
-Scheduler::~Scheduler() = default;
+Scheduler::~Scheduler() {
+ // Ensure the IdleTimer thread is joined before we start destroying state.
+ mIdleTimer.reset();
+}
sp<Scheduler::ConnectionHandle> Scheduler::createConnection(
const char* connectionName, int64_t phaseOffsetNs, ResyncCallback resyncCallback,
diff --git a/services/surfaceflinger/tests/unittests/IdleTimerTest.cpp b/services/surfaceflinger/tests/unittests/IdleTimerTest.cpp
index 9fe9a18..dc63260 100644
--- a/services/surfaceflinger/tests/unittests/IdleTimerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/IdleTimerTest.cpp
@@ -34,91 +34,117 @@
IdleTimerTest() = default;
~IdleTimerTest() override = default;
+ // This timeout should be used when a 3ms callback is expected.
+ // While the tests typically request a callback after 3ms, the scheduler
+ // does not always cooperate, at it can take significantly longer (observed
+ // 30ms).
+ static constexpr auto waitTimeForExpected3msCallback = 100ms;
+
+ // This timeout should be used when an 3ms callback is not expected.
+ // Note that there can be false-negatives if the callback happens later.
+ static constexpr auto waitTimeForUnexpected3msCallback = 6ms;
+
AsyncCallRecorder<void (*)()> mExpiredTimerCallback;
std::unique_ptr<IdleTimer> mIdleTimer;
+
+ void clearPendingCallbacks() {
+ while (mExpiredTimerCallback.waitForCall(0us).has_value()) {
+ }
+ }
};
namespace {
TEST_F(IdleTimerTest, createAndDestroyTest) {
- mIdleTimer = std::make_unique<scheduler::IdleTimer>(30ms, [] {});
+ mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, [] {});
}
TEST_F(IdleTimerTest, startStopTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(30ms, mExpiredTimerCallback.getInvocable());
+ auto startTime = std::chrono::steady_clock::now();
mIdleTimer->start();
- std::this_thread::sleep_for(std::chrono::milliseconds(10));
- // The timer expires after 30 ms, so the call to the callback should not happen.
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall().has_value());
+ // The idle timer fires after 30ms, so there should be no callback within
+ // 25ms (waiting for a ballback for the full 30ms would be problematic).
+ bool callbackCalled = mExpiredTimerCallback.waitForCall(25ms).has_value();
+ // Under ideal conditions there should be no event. But occasionally
+ // it is possible that the wait just prior takes more than 30ms, and
+ // a callback is observed. We check the elapsed time since before the IdleTimer
+ // thread was started as a sanity check to not have a flakey test.
+ EXPECT_FALSE(callbackCalled && std::chrono::steady_clock::now() - startTime < 30ms);
mIdleTimer->stop();
}
TEST_F(IdleTimerTest, resetTest) {
- mIdleTimer = std::make_unique<scheduler::IdleTimer>(20ms, mExpiredTimerCallback.getInvocable());
+ mIdleTimer = std::make_unique<scheduler::IdleTimer>(30ms, mExpiredTimerCallback.getInvocable());
mIdleTimer->start();
- std::this_thread::sleep_for(std::chrono::milliseconds(10));
- // The timer expires after 30 ms, so the call to the callback should not happen.
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ // Observe any event that happens in about 25ms. We don't care if one was
+ // observed or not.
+ mExpiredTimerCallback.waitForCall(25ms).has_value();
mIdleTimer->reset();
- // The timer was reset, so the call to the callback should not happen.
- std::this_thread::sleep_for(std::chrono::milliseconds(15));
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ // There may have been a race with the reset. Clear any callbacks we
+ // received right afterwards.
+ clearPendingCallbacks();
+ // A single callback should be generated after 30ms
+ EXPECT_TRUE(
+ mExpiredTimerCallback.waitForCall(waitTimeForExpected3msCallback + 30ms).has_value());
+ // After one event, it should be idle, and not generate another.
+ EXPECT_FALSE(
+ mExpiredTimerCallback.waitForCall(waitTimeForUnexpected3msCallback * 10).has_value());
mIdleTimer->stop();
+ // Final quick check that no more callback were observed.
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(0ms).has_value());
}
TEST_F(IdleTimerTest, startNotCalledTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, mExpiredTimerCallback.getInvocable());
- std::this_thread::sleep_for(6ms);
// The start hasn't happened, so the callback does not happen.
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(waitTimeForUnexpected3msCallback).has_value());
mIdleTimer->stop();
+ // Final quick check that no more callback were observed.
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(0ms).has_value());
}
TEST_F(IdleTimerTest, idleTimerIdlesTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, mExpiredTimerCallback.getInvocable());
mIdleTimer->start();
- std::this_thread::sleep_for(6ms);
- // The timer expires after 3 ms, so the call to the callback happens.
- EXPECT_TRUE(mExpiredTimerCallback.waitForCall(1us).has_value());
- std::this_thread::sleep_for(6ms);
- // Timer can be idle.
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall(1us).has_value());
- // Timer can be reset.
+ // A callback should be generated after 3ms
+ EXPECT_TRUE(mExpiredTimerCallback.waitForCall(waitTimeForExpected3msCallback).has_value());
+ // After one event, it should be idle, and not generate another.
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(waitTimeForUnexpected3msCallback).has_value());
+ // Once reset, it should generate another
mIdleTimer->reset();
- std::this_thread::sleep_for(6ms);
- // Timer fires again.
- EXPECT_TRUE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ EXPECT_TRUE(mExpiredTimerCallback.waitForCall(waitTimeForExpected3msCallback).has_value());
mIdleTimer->stop();
+ // Final quick check that no more callback were observed.
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(0ms).has_value());
}
TEST_F(IdleTimerTest, timeoutCallbackExecutionTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, mExpiredTimerCallback.getInvocable());
mIdleTimer->start();
- std::this_thread::sleep_for(6ms);
- // The timer expires after 3 ms, so the call to the callback should happen.
- EXPECT_TRUE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ EXPECT_TRUE(mExpiredTimerCallback.waitForCall(waitTimeForExpected3msCallback).has_value());
mIdleTimer->stop();
}
TEST_F(IdleTimerTest, noCallbacksAfterStopAndResetTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, mExpiredTimerCallback.getInvocable());
mIdleTimer->start();
- std::this_thread::sleep_for(6ms);
- EXPECT_TRUE(mExpiredTimerCallback.waitForCall().has_value());
+ EXPECT_TRUE(mExpiredTimerCallback.waitForCall(waitTimeForExpected3msCallback).has_value());
mIdleTimer->stop();
+ clearPendingCallbacks();
mIdleTimer->reset();
- std::this_thread::sleep_for(6ms);
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall().has_value());
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(waitTimeForUnexpected3msCallback).has_value());
}
TEST_F(IdleTimerTest, noCallbacksAfterStopTest) {
mIdleTimer = std::make_unique<scheduler::IdleTimer>(3ms, mExpiredTimerCallback.getInvocable());
mIdleTimer->start();
- std::this_thread::sleep_for(1ms);
mIdleTimer->stop();
- std::this_thread::sleep_for(3ms);
- EXPECT_FALSE(mExpiredTimerCallback.waitForCall(1us).has_value());
+ clearPendingCallbacks();
+ mIdleTimer->reset();
+ // No more idle events should be observed
+ EXPECT_FALSE(mExpiredTimerCallback.waitForCall(waitTimeForUnexpected3msCallback).has_value());
}
} // namespace
diff --git a/services/surfaceflinger/tests/unittests/libsurfaceflinger_unittest_main.cpp b/services/surfaceflinger/tests/unittests/libsurfaceflinger_unittest_main.cpp
index bc1f00d..ed628b8 100644
--- a/services/surfaceflinger/tests/unittests/libsurfaceflinger_unittest_main.cpp
+++ b/services/surfaceflinger/tests/unittests/libsurfaceflinger_unittest_main.cpp
@@ -16,6 +16,11 @@
#include <gtest/gtest.h>
+#include <sched.h>
+
+#include <processgroup/sched_policy.h>
+#include <system/graphics.h>
+
#include "libsurfaceflinger_unittest_main.h"
// ------------------------------------------------------------------------
@@ -39,6 +44,12 @@
int main(int argc, char **argv) {
::testing::InitGoogleTest(&argc, argv);
+ // The SurfaceFlinger implementation assumes that threads resume
+ // execution as quickly as possible once they become unblocked.
+ // (These same calls are made in main_surfaceflinger.cpp)
+ setpriority(PRIO_PROCESS, 0, HAL_PRIORITY_URGENT_DISPLAY);
+ set_sched_policy(0, SP_FOREGROUND);
+
for (int i = 1; i < argc; i++) {
if (strcmp(argv[i], "--no-slow") == 0) {
g_noSlowTests = true;