From 15950eec373e8b06123edae22b5488ed3bb4fe55 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Sat, 5 Mar 2022 14:40:04 +0100 Subject: [PATCH] [Base] Use chrono APIs for Timers --- src/xenia/base/testing/threading_test.cc | 28 ++-- src/xenia/base/threading.h | 36 +++-- src/xenia/base/threading_posix.cc | 189 ++++++++++------------- src/xenia/base/threading_win.cc | 42 ++++- src/xenia/kernel/xtimer.cc | 23 ++- 5 files changed, 170 insertions(+), 148 deletions(-) diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index 2643e2148..6d8f4f3c8 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -11,6 +11,7 @@ #include "xenia/base/threading.h" +#define CATCH_CONFIG_ENABLE_CHRONO_STRINGMAKER #include "third_party/catch/include/catch.hpp" namespace xe { @@ -786,7 +787,7 @@ TEST_CASE("Wait on Timer", "[timer]") { REQUIRE(timer); result = Wait(timer.get(), false, 1ms); REQUIRE(result == WaitResult::kTimeout); - REQUIRE(timer->SetOnce(1ms)); // Signals it + REQUIRE(timer->SetOnceAfter(1ms)); // Signals it result = Wait(timer.get(), false, 2ms); REQUIRE(result == WaitResult::kSuccess); result = Wait(timer.get(), false, 1ms); @@ -797,21 +798,20 @@ TEST_CASE("Wait on Timer", "[timer]") { REQUIRE(timer); result = Wait(timer.get(), false, 1ms); REQUIRE(result == WaitResult::kTimeout); - REQUIRE(timer->SetOnce(1ms)); // Signals it + REQUIRE(timer->SetOnceAfter(1ms)); // Signals it result = Wait(timer.get(), false, 2ms); REQUIRE(result == WaitResult::kSuccess); result = Wait(timer.get(), false, 1ms); REQUIRE(result == WaitResult::kTimeout); // Did reset - // TODO(bwrsandman): This test unexpectedly fails under windows // Test long due time - // timer = Timer::CreateSynchronizationTimer(); - // REQUIRE(timer->SetOnce(10s)); - // result = Wait(timer.get(), false, 10ms); // Still signals under windows - // REQUIRE(result == WaitResult::kTimeout); + timer = Timer::CreateSynchronizationTimer(); + REQUIRE(timer->SetOnceAfter(10s)); + result = Wait(timer.get(), false, 10ms); + REQUIRE(result == WaitResult::kTimeout); // Test Repeating - REQUIRE(timer->SetRepeating(1ms, 10ms)); + REQUIRE(timer->SetRepeatingAfter(1ms, 10ms)); for (int i = 0; i < 10; ++i) { result = Wait(timer.get(), false, 20ms); INFO(i); @@ -832,12 +832,12 @@ TEST_CASE("Wait on Timer", "[timer]") { result = Wait(timer.get(), false, 20ms); REQUIRE(result == WaitResult::kTimeout); // Cancel with SetOnce - REQUIRE(timer->SetRepeating(1ms, 10ms)); + REQUIRE(timer->SetRepeatingAfter(1ms, 10ms)); for (int i = 0; i < 10; ++i) { result = Wait(timer.get(), false, 20ms); REQUIRE(result == WaitResult::kSuccess); } - REQUIRE(timer->SetOnce(1ms)); + REQUIRE(timer->SetOnceAfter(1ms)); result = Wait(timer.get(), false, 20ms); REQUIRE(result == WaitResult::kSuccess); // Signal from Set Once result = Wait(timer.get(), false, 20ms); @@ -859,7 +859,7 @@ TEST_CASE("Wait on Multiple Timers", "[timer]") { REQUIRE(any_result.second == 0); // Some signaled - REQUIRE(timer1->SetOnce(1ms)); + REQUIRE(timer1->SetOnceAfter(1ms)); all_result = WaitAll({timer0.get(), timer1.get()}, false, 100ms); REQUIRE(all_result == WaitResult::kTimeout); any_result = WaitAny({timer0.get(), timer1.get()}, false, 100ms); @@ -867,11 +867,11 @@ TEST_CASE("Wait on Multiple Timers", "[timer]") { REQUIRE(any_result.second == 1); // All signaled - REQUIRE(timer0->SetOnce(1ms)); + REQUIRE(timer0->SetOnceAfter(1ms)); all_result = WaitAll({timer0.get(), timer1.get()}, false, 100ms); REQUIRE(all_result == WaitResult::kSuccess); - REQUIRE(timer0->SetOnce(1ms)); - Sleep(1ms); + REQUIRE(timer0->SetOnceAfter(1ms)); + Sleep(2ms); any_result = WaitAny({timer0.get(), timer1.get()}, false, 100ms); REQUIRE(any_result.first == WaitResult::kSuccess); REQUIRE(any_result.second == 0); diff --git a/src/xenia/base/threading.h b/src/xenia/base/threading.h index 08c34cd7d..28d9a780e 100644 --- a/src/xenia/base/threading.h +++ b/src/xenia/base/threading.h @@ -25,6 +25,7 @@ #include #include "xenia/base/assert.h" +#include "xenia/base/chrono.h" #include "xenia/base/literals.h" #include "xenia/base/platform.h" #include "xenia/base/threading_timer_queue.h" @@ -338,6 +339,13 @@ class Mutant : public WaitHandle { // https://msdn.microsoft.com/en-us/library/windows/desktop/ms687012(v=vs.85).aspx class Timer : public WaitHandle { public: + // Make vtable entries for both so we can defer conversions and only do them + // if really necessary (let the calling code what clock it prefers). Windows + // kernel sync primitives will work with WinSystemClock while our own + // implementation works with steady_clock. + using WClock_ = xe::chrono::WinSystemClock; + using GClock_ = std::chrono::steady_clock; // generic + // Creates a timer whose state remains signaled until SetOnce() or // SetRepeating() is called to establish a new due time. static std::unique_ptr CreateManualResetTimer(); @@ -350,25 +358,27 @@ class Timer : public WaitHandle { // timer is signaled and the thread that set the timer calls the optional // completion routine. // Returns true on success. - virtual bool SetOnce(std::chrono::nanoseconds due_time, - std::function opt_callback = nullptr) = 0; + virtual bool SetOnceAfter(xe::chrono::hundrednanoseconds rel_time, + std::function opt_callback = nullptr) = 0; + virtual bool SetOnceAt(WClock_::time_point due_time, + std::function opt_callback = nullptr) = 0; + virtual bool SetOnceAt(GClock_::time_point due_time, + std::function opt_callback = nullptr) = 0; // Activates the specified waitable timer. When the due time arrives, the // timer is signaled and the thread that set the timer calls the optional // completion routine. A periodic timer automatically reactivates each time // the period elapses, until the timer is canceled or reset. // Returns true on success. - virtual bool SetRepeating(std::chrono::nanoseconds due_time, - std::chrono::milliseconds period, - std::function opt_callback = nullptr) = 0; - template - bool SetRepeating(std::chrono::nanoseconds due_time, - std::chrono::duration period, - std::function opt_callback = nullptr) { - return SetRepeating( - due_time, std::chrono::duration_cast(period), - std::move(opt_callback)); - } + virtual bool SetRepeatingAfter( + xe::chrono::hundrednanoseconds rel_time, std::chrono::milliseconds period, + std::function opt_callback = nullptr) = 0; + virtual bool SetRepeatingAt(WClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback = nullptr) = 0; + virtual bool SetRepeatingAt(GClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback = nullptr) = 0; // Stops the timer before it can be set to the signaled state and cancels // outstanding callbacks. Threads performing a wait operation on the timer diff --git a/src/xenia/base/threading_posix.cc b/src/xenia/base/threading_posix.cc index e341eb9d8..bf7e0cf36 100644 --- a/src/xenia/base/threading_posix.cc +++ b/src/xenia/base/threading_posix.cc @@ -10,8 +10,10 @@ #include "xenia/base/threading.h" #include "xenia/base/assert.h" +#include "xenia/base/chrono_steady_cast.h" #include "xenia/base/delay_scheduler.h" #include "xenia/base/platform.h" +#include "xenia/base/threading_timer_queue.h" #include #include @@ -133,8 +135,6 @@ inline timespec DurationToTimeSpec( // gdb tip, for SIG = SIGRTMIN + SignalType : handle SIG nostop // lldb tip, for SIG = SIGRTMIN + SignalType : process handle SIG -s false enum class SignalType { - kHighResolutionTimer, - kTimer, kThreadSuspend, kThreadUserCallback, #if XE_PLATFORM_ANDROID @@ -430,10 +430,7 @@ template <> class PosixCondition : public PosixConditionBase { public: explicit PosixCondition(bool manual_reset) - : timer_(nullptr), - callback_info_(nullptr), - signal_(false), - manual_reset_(manual_reset) {} + : callback_(nullptr), signal_(false), manual_reset_(manual_reset) {} virtual ~PosixCondition() { Cancel(); } @@ -444,58 +441,55 @@ class PosixCondition : public PosixConditionBase { return true; } - // TODO(bwrsandman): due_times of under 1ms deadlock under travis - // TODO(joellinn): This is likely due to deadlock on mutex_ if Signal() is - // called from signal_handler running in Thread A while thread A was still in - // Set(...) routine inside the lock - bool Set(std::chrono::nanoseconds due_time, std::chrono::milliseconds period, - std::function opt_callback = nullptr) { + void SetOnce(std::chrono::steady_clock::time_point due_time, + std::function opt_callback) { Cancel(); std::lock_guard lock(mutex_); - callback_info_ = new timer_callback_info_t(std::move(opt_callback)); - callback_info_->userdata = this; + + callback_ = std::move(opt_callback); signal_ = false; - - // Create timer - sigevent sev{}; -#if XE_HAS_SIGEV_THREAD_ID - sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID; - sev.sigev_notify_thread_id = gettid(); -#else - sev.sigev_notify = SIGEV_SIGNAL; - callback_info_->target_thread = pthread_self(); -#endif - sev.sigev_signo = GetSystemSignal(SignalType::kTimer); - sev.sigev_value.sival_ptr = callback_info_; - if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) { - delete callback_info_; - return false; - } - - // Start timer - itimerspec its{}; - its.it_value = DurationToTimeSpec(due_time); - its.it_interval = DurationToTimeSpec(period); - return timer_settime(timer_, 0, &its, nullptr) == 0; + wait_item_ = QueueTimerOnce(&CompletionRoutine, this, due_time); } - bool Cancel() { + void SetRepeating(std::chrono::steady_clock::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback) { + Cancel(); + std::lock_guard lock(mutex_); - bool result = true; - if (timer_) { - callback_info_->disarmed = true; - result = timer_delete(timer_) == 0; - timer_ = nullptr; - static_cast(timers_garbage_collector_.TryScheduleAfter( - callback_info_, timers_garbage_collector_delay)); - callback_info_ = nullptr; + + callback_ = std::move(opt_callback); + signal_ = false; + wait_item_ = + QueueTimerRecurring(&CompletionRoutine, this, due_time, period); + } + + void Cancel() { + if (auto wait_item = wait_item_.lock()) { + wait_item->Disarm(); } - return result; } void* native_handle() const override { - return reinterpret_cast(timer_); + assert_always(); + return nullptr; + } + + private: + static void CompletionRoutine(void* userdata) { + assert_not_null(userdata); + auto timer = reinterpret_cast*>(userdata); + timer->Signal(); + // As the callback may reset the timer, store local. + std::function callback; + { + std::lock_guard lock(timer->mutex_); + callback = timer->callback_; + } + if (callback) { + callback(); + } } private: @@ -505,8 +499,8 @@ class PosixCondition : public PosixConditionBase { signal_ = false; } } - timer_t timer_; - timer_callback_info_t* callback_info_; + std::weak_ptr wait_item_; + std::function callback_; volatile bool signal_; const bool manual_reset_; }; @@ -1007,29 +1001,57 @@ std::unique_ptr Mutant::Create(bool initial_owner) { } class PosixTimer : public PosixConditionHandle { + using WClock_ = Timer::WClock_; + using GClock_ = Timer::GClock_; + public: explicit PosixTimer(bool manual_reset) : PosixConditionHandle(manual_reset) {} ~PosixTimer() override = default; - bool SetOnce(std::chrono::nanoseconds due_time, - std::function opt_callback) override { - return handle_.Set(due_time, std::chrono::milliseconds::zero(), - std::move(opt_callback)); + + bool SetOnceAfter(xe::chrono::hundrednanoseconds rel_time, + std::function opt_callback = nullptr) override { + return SetOnceAt(GClock_::now() + rel_time, std::move(opt_callback)); } - bool SetRepeating(std::chrono::nanoseconds due_time, - std::chrono::milliseconds period, - std::function opt_callback) override { - return handle_.Set(due_time, period, std::move(opt_callback)); + bool SetOnceAt(WClock_::time_point due_time, + std::function opt_callback = nullptr) override { + return SetOnceAt(date::clock_cast(due_time), + std::move(opt_callback)); + }; + bool SetOnceAt(GClock_::time_point due_time, + std::function opt_callback = nullptr) override { + handle_.SetOnce(due_time, std::move(opt_callback)); + return true; + } + + bool SetRepeatingAfter( + xe::chrono::hundrednanoseconds rel_time, std::chrono::milliseconds period, + std::function opt_callback = nullptr) override { + return SetRepeatingAt(GClock_::now() + rel_time, period, + std::move(opt_callback)); + } + bool SetRepeatingAt(WClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback = nullptr) override { + return SetRepeatingAt(date::clock_cast(due_time), period, + std::move(opt_callback)); + } + bool SetRepeatingAt(GClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback = nullptr) override { + handle_.SetRepeating(due_time, period, std::move(opt_callback)); + return true; + } + bool Cancel() override { + handle_.Cancel(); + return true; } - bool Cancel() override { return handle_.Cancel(); } }; std::unique_ptr Timer::CreateManualResetTimer() { - install_signal_handler(SignalType::kTimer); return std::make_unique(true); } std::unique_ptr Timer::CreateSynchronizationTimer() { - install_signal_handler(SignalType::kTimer); return std::make_unique(false); } @@ -1187,53 +1209,6 @@ void set_name(const std::string_view name) { static void signal_handler(int signal, siginfo_t* info, void* /*context*/) { switch (GetSystemSignalType(signal)) { - case SignalType::kHighResolutionTimer: { - assert_not_null(info->si_value.sival_ptr); - auto timer_info = - reinterpret_cast(info->si_value.sival_ptr); - if (!timer_info->disarmed) { -#if XE_HAS_SIGEV_THREAD_ID - { -#else - if (pthread_self() != timer_info->target_thread) { - sigval info_inner{}; - info_inner.sival_ptr = timer_info; - const auto queueres = pthread_sigqueue( - timer_info->target_thread, - GetSystemSignal(SignalType::kHighResolutionTimer), info_inner); - assert_zero(queueres); - } else { -#endif - timer_info->callback(); - } - } - } break; - case SignalType::kTimer: { - assert_not_null(info->si_value.sival_ptr); - auto timer_info = - reinterpret_cast(info->si_value.sival_ptr); - if (!timer_info->disarmed) { - assert_not_null(timer_info->userdata); - auto timer = static_cast*>(timer_info->userdata); -#if XE_HAS_SIGEV_THREAD_ID - { -#else - if (pthread_self() != timer_info->target_thread) { - sigval info_inner{}; - info_inner.sival_ptr = timer_info; - const auto queueres = - pthread_sigqueue(timer_info->target_thread, - GetSystemSignal(SignalType::kTimer), info_inner); - assert_zero(queueres); - } else { -#endif - timer->Signal(); - if (timer_info->callback) { - timer_info->callback(); - } - } - } - } break; case SignalType::kThreadSuspend: { assert_not_null(current_thread_); current_thread_->WaitSuspended(); diff --git a/src/xenia/base/threading_win.cc b/src/xenia/base/threading_win.cc index 60a3f7843..8f6087b05 100644 --- a/src/xenia/base/threading_win.cc +++ b/src/xenia/base/threading_win.cc @@ -8,6 +8,7 @@ */ #include "xenia/base/assert.h" +#include "xenia/base/chrono_steady_cast.h" #include "xenia/base/logging.h" #include "xenia/base/platform_win.h" #include "xenia/base/threading.h" @@ -276,15 +277,28 @@ std::unique_ptr Mutant::Create(bool initial_owner) { } class Win32Timer : public Win32Handle { + using WClock_ = Timer::WClock_; + using GClock_ = Timer::GClock_; + public: explicit Win32Timer(HANDLE handle) : Win32Handle(handle) {} ~Win32Timer() = default; - bool SetOnce(std::chrono::nanoseconds due_time, - std::function opt_callback) override { + + bool SetOnceAfter(xe::chrono::hundrednanoseconds rel_time, + std::function opt_callback) override { + return SetOnceAt(WClock_::now() + rel_time, std::move(opt_callback)); + } + bool SetOnceAt(GClock_::time_point due_time, + std::function opt_callback) override { + return SetOnceAt(date::clock_cast(due_time), + std::move(opt_callback)); + } + bool SetOnceAt(WClock_::time_point due_time, + std::function opt_callback) override { std::lock_guard lock(mutex_); callback_ = std::move(opt_callback); LARGE_INTEGER due_time_li; - due_time_li.QuadPart = due_time.count() / 100; + due_time_li.QuadPart = WClock_::to_file_time(due_time); auto completion_routine = callback_ ? reinterpret_cast(CompletionRoutine) : NULL; @@ -293,13 +307,26 @@ class Win32Timer : public Win32Handle { ? true : false; } - bool SetRepeating(std::chrono::nanoseconds due_time, - std::chrono::milliseconds period, - std::function opt_callback) override { + + bool SetRepeatingAfter( + xe::chrono::hundrednanoseconds rel_time, std::chrono::milliseconds period, + std::function opt_callback = nullptr) override { + return SetRepeatingAt(WClock_::now() + rel_time, period, + std::move(opt_callback)); + } + bool SetRepeatingAt(GClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback = nullptr) { + return SetRepeatingAt(date::clock_cast(due_time), period, + std::move(opt_callback)); + } + bool SetRepeatingAt(WClock_::time_point due_time, + std::chrono::milliseconds period, + std::function opt_callback) override { std::lock_guard lock(mutex_); callback_ = std::move(opt_callback); LARGE_INTEGER due_time_li; - due_time_li.QuadPart = due_time.count() / 100; + due_time_li.QuadPart = WClock_::to_file_time(due_time); auto completion_routine = callback_ ? reinterpret_cast(CompletionRoutine) : NULL; @@ -308,6 +335,7 @@ class Win32Timer : public Win32Handle { ? true : false; } + bool Cancel() override { // Reset the callback immediately so that any completions don't call it. std::lock_guard lock(mutex_); diff --git a/src/xenia/kernel/xtimer.cc b/src/xenia/kernel/xtimer.cc index e98add6d3..b5b9530fa 100644 --- a/src/xenia/kernel/xtimer.cc +++ b/src/xenia/kernel/xtimer.cc @@ -9,7 +9,7 @@ #include "xenia/kernel/xtimer.h" -#include "xenia/base/clock.h" +#include "xenia/base/chrono.h" #include "xenia/base/logging.h" #include "xenia/cpu/processor.h" #include "xenia/kernel/xthread.h" @@ -40,13 +40,24 @@ void XTimer::Initialize(uint32_t timer_type) { X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, uint32_t routine, uint32_t routine_arg, bool resume) { + using xe::chrono::WinSystemClock; + using xe::chrono::XSystemClock; // Caller is checking for STATUS_TIMER_RESUME_IGNORED. if (resume) { return X_STATUS_TIMER_RESUME_IGNORED; } - due_time = Clock::ScaleGuestDurationFileTime(due_time); period_ms = Clock::ScaleGuestDurationMillis(period_ms); + WinSystemClock::time_point due_tp; + if (due_time < 0) { + // Any timer implementation uses absolute times eventually, convert as early + // as possible for increased accuracy + auto after = xe::chrono::hundrednanoseconds(-due_time); + due_tp = date::clock_cast(XSystemClock::now() + after); + } else { + due_tp = date::clock_cast( + XSystemClock::from_file_time(due_time)); + } // Stash routine for callback. callback_thread_ = XThread::GetCurrentThread(); @@ -72,12 +83,10 @@ X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, bool result; if (!period_ms) { - result = timer_->SetOnce(std::chrono::nanoseconds(due_time * 100), - std::move(callback)); + result = timer_->SetOnceAt(due_tp, std::move(callback)); } else { - result = timer_->SetRepeating(std::chrono::nanoseconds(due_time * 100), - std::chrono::milliseconds(period_ms), - std::move(callback)); + result = timer_->SetRepeatingAt( + due_tp, std::chrono::milliseconds(period_ms), std::move(callback)); } return result ? X_STATUS_SUCCESS : X_STATUS_UNSUCCESSFUL;