From 1d5be49ff29bb322ae19b16f09976c699e09113b Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Thu, 18 Aug 2022 13:49:09 +0000 Subject: [PATCH] rtc::Event: Add TimeDelta support. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL adds TimeDelta support to the rtc::Event, and updates the Wait implementations to work with the improved precision. Bug: webrtc:14366 Change-Id: Iefeb638b18176a34f4ed2a5131754a7b7e6c9e99 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272002 Reviewed-by: Danil Chapovalov Reviewed-by: Erik Språng Reviewed-by: Mirko Bonadei Commit-Queue: Markus Handell Cr-Commit-Position: refs/heads/main@{#37831} --- rtc_base/BUILD.gn | 2 + rtc_base/event.cc | 35 +++++++++------- rtc_base/event.h | 40 ++++++++++++++----- rtc_base/event_unittest.cc | 10 +++++ rtc_base/null_socket_server.cc | 6 ++- rtc_base/task_queue_for_test.h | 4 +- .../time_controller_conformance_test.cc | 3 +- 7 files changed, 72 insertions(+), 28 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index e96e6616a5..a13864ad77 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -392,6 +392,8 @@ rtc_library("rtc_event") { ] deps = [ ":checks", + ":timeutils", + "../api/units:time_delta", "synchronization:yield_policy", "system:warn_current_thread_is_deadlocked", ] diff --git a/rtc_base/event.cc b/rtc_base/event.cc index 67c8746205..c2f6f8abab 100644 --- a/rtc_base/event.cc +++ b/rtc_base/event.cc @@ -25,9 +25,12 @@ #include "rtc_base/checks.h" #include "rtc_base/synchronization/yield_policy.h" #include "rtc_base/system/warn_current_thread_is_deadlocked.h" +#include "rtc_base/time_utils.h" namespace rtc { +using ::webrtc::TimeDelta; + Event::Event() : Event(false, false) {} #if defined(WEBRTC_WIN) @@ -51,9 +54,12 @@ void Event::Reset() { ResetEvent(event_handle_); } -bool Event::Wait(const int give_up_after_ms, int /*warn_after_ms*/) { +bool Event::Wait(TimeDelta give_up_after, TimeDelta /*warn_after*/) { ScopedYieldPolicy::YieldExecution(); - const DWORD ms = give_up_after_ms == kForever ? INFINITE : give_up_after_ms; + const DWORD ms = + give_up_after.IsPlusInfinity() + ? INFINITE + : give_up_after.RoundUpTo(webrtc::TimeDelta::Millis(1)).ms(); return (WaitForSingleObject(event_handle_, ms) == WAIT_OBJECT_0); } @@ -108,7 +114,7 @@ void Event::Reset() { namespace { -timespec GetTimespec(const int milliseconds_from_now) { +timespec GetTimespec(TimeDelta duration_from_now) { timespec ts; // Get the current time. @@ -118,17 +124,19 @@ timespec GetTimespec(const int milliseconds_from_now) { timeval tv; gettimeofday(&tv, nullptr); ts.tv_sec = tv.tv_sec; - ts.tv_nsec = tv.tv_usec * 1000; + ts.tv_nsec = tv.tv_usec * kNumNanosecsPerMicrosec; #endif // Add the specified number of milliseconds to it. - ts.tv_sec += (milliseconds_from_now / 1000); - ts.tv_nsec += (milliseconds_from_now % 1000) * 1000000; + int64_t microsecs_from_now = duration_from_now.us(); + ts.tv_sec += microsecs_from_now / kNumMicrosecsPerSec; + ts.tv_nsec += + (microsecs_from_now % kNumMicrosecsPerSec) * kNumNanosecsPerMicrosec; // Normalize. - if (ts.tv_nsec >= 1000000000) { + if (ts.tv_nsec >= kNumNanosecsPerSec) { ts.tv_sec++; - ts.tv_nsec -= 1000000000; + ts.tv_nsec -= kNumNanosecsPerSec; } return ts; @@ -136,22 +144,21 @@ timespec GetTimespec(const int milliseconds_from_now) { } // namespace -bool Event::Wait(const int give_up_after_ms, const int warn_after_ms) { +bool Event::Wait(TimeDelta give_up_after, TimeDelta warn_after) { // Instant when we'll log a warning message (because we've been waiting so // long it might be a bug), but not yet give up waiting. nullopt if we // shouldn't log a warning. const absl::optional warn_ts = - warn_after_ms == kForever || - (give_up_after_ms != kForever && warn_after_ms > give_up_after_ms) + warn_after >= give_up_after ? absl::nullopt - : absl::make_optional(GetTimespec(warn_after_ms)); + : absl::make_optional(GetTimespec(warn_after)); // Instant when we'll stop waiting and return an error. nullopt if we should // never give up. const absl::optional give_up_ts = - give_up_after_ms == kForever + give_up_after.IsPlusInfinity() ? absl::nullopt - : absl::make_optional(GetTimespec(give_up_after_ms)); + : absl::make_optional(GetTimespec(give_up_after)); ScopedYieldPolicy::YieldExecution(); pthread_mutex_lock(&event_mutex_); diff --git a/rtc_base/event.h b/rtc_base/event.h index 584ad5d35a..ab66d6afce 100644 --- a/rtc_base/event.h +++ b/rtc_base/event.h @@ -11,6 +11,7 @@ #ifndef RTC_BASE_EVENT_H_ #define RTC_BASE_EVENT_H_ +#include "api/units/time_delta.h" #if defined(WEBRTC_WIN) #include #elif defined(WEBRTC_POSIX) @@ -23,7 +24,9 @@ namespace rtc { class Event { public: - static const int kForever = -1; + // TODO(bugs.webrtc.org/14366): Consider removing this redundant alias. + static constexpr webrtc::TimeDelta kForever = + webrtc::TimeDelta::PlusInfinity(); Event(); Event(bool manual_reset, bool initially_signaled); @@ -35,22 +38,41 @@ class Event { void Reset(); // Waits for the event to become signaled, but logs a warning if it takes more - // than `warn_after_ms` milliseconds, and gives up completely if it takes more - // than `give_up_after_ms` milliseconds. (If `warn_after_ms >= - // give_up_after_ms`, no warning will be logged.) Either or both may be - // `kForever`, which means wait indefinitely. + // than `warn_after`, and gives up completely if it takes more than + // `give_up_after`. (If `warn_after >= give_up_after`, no warning will be + // logged.) Either or both may be `kForever`, which means wait indefinitely. + // + // Care is taken so that the underlying OS wait call isn't requested to sleep + // shorter than `give_up_after`. // // Returns true if the event was signaled, false if there was a timeout or // some other error. - bool Wait(int give_up_after_ms, int warn_after_ms); + bool Wait(webrtc::TimeDelta give_up_after, webrtc::TimeDelta warn_after); // Waits with the given timeout and a reasonable default warning timeout. - bool Wait(int give_up_after_ms) { - return Wait(give_up_after_ms, - give_up_after_ms == kForever ? 3000 : kForever); + // TODO(bugs.webrtc.org/14366): De-template this after millisec-based Wait is + // removed. + template + bool Wait(T give_up_after) { + webrtc::TimeDelta duration = ToTimeDelta(give_up_after); + return Wait(duration, duration.IsPlusInfinity() + ? webrtc::TimeDelta::Seconds(3) + : kForever); } private: + // TODO(bugs.webrtc.org/14366): Remove after millisec-based Wait is removed. + static webrtc::TimeDelta ToTimeDelta(int duration) { + // SocketServer users can get here with SocketServer::kForever which is + // -1. Mirror the definition here to avoid dependence. + constexpr int kForeverMs = -1; + return duration == kForeverMs ? kForever + : webrtc::TimeDelta::Millis(duration); + } + static webrtc::TimeDelta ToTimeDelta(webrtc::TimeDelta duration) { + return duration; + } + #if defined(WEBRTC_WIN) HANDLE event_handle_; #elif defined(WEBRTC_POSIX) diff --git a/rtc_base/event_unittest.cc b/rtc_base/event_unittest.cc index a634d6e426..099665abd8 100644 --- a/rtc_base/event_unittest.cc +++ b/rtc_base/event_unittest.cc @@ -11,6 +11,7 @@ #include "rtc_base/event.h" #include "rtc_base/platform_thread.h" +#include "system_wrappers/include/clock.h" #include "test/gtest.h" namespace rtc { @@ -65,6 +66,15 @@ class SignalerThread { PlatformThread thread_; }; +TEST(EventTest, UnsignaledWaitDoesNotReturnBeforeTimeout) { + constexpr webrtc::TimeDelta kDuration = webrtc::TimeDelta::Micros(10'499); + Event event; + auto begin = webrtc::Clock::GetRealTimeClock()->CurrentTime(); + EXPECT_FALSE(event.Wait(kDuration)); + EXPECT_GE(webrtc::Clock::GetRealTimeClock()->CurrentTime(), + begin + kDuration); +} + // These tests are disabled by default and only intended to be run manually. TEST(EventTest, DISABLED_PerformanceSingleThread) { static const int kNumIterations = 10000000; diff --git a/rtc_base/null_socket_server.cc b/rtc_base/null_socket_server.cc index 4705163c4a..32a6215e31 100644 --- a/rtc_base/null_socket_server.cc +++ b/rtc_base/null_socket_server.cc @@ -11,6 +11,7 @@ #include "rtc_base/null_socket_server.h" #include "rtc_base/checks.h" +#include "rtc_base/event.h" namespace rtc { @@ -21,7 +22,10 @@ bool NullSocketServer::Wait(int cms, bool process_io) { // Wait with the given timeout. Do not log a warning if we end up waiting for // a long time; that just means no one has any work for us, which is perfectly // legitimate. - event_.Wait(/*give_up_after_ms=*/cms, /*warn_after_ms=*/Event::kForever); + event_.Wait(/*give_up_after=*/cms == kForever + ? Event::kForever + : webrtc::TimeDelta::Millis(cms), + /*warn_after_ms=*/Event::kForever); return true; } diff --git a/rtc_base/task_queue_for_test.h b/rtc_base/task_queue_for_test.h index 616ec8e2a5..4c7f842abe 100644 --- a/rtc_base/task_queue_for_test.h +++ b/rtc_base/task_queue_for_test.h @@ -34,8 +34,8 @@ inline void SendTask(TaskQueueBase* task_queue, rtc::Event event; absl::Cleanup cleanup = [&event] { event.Set(); }; task_queue->PostTask([task, cleanup = std::move(cleanup)] { task(); }); - RTC_CHECK(event.Wait(/*give_up_after_ms=*/rtc::Event::kForever, - /*warn_after_ms=*/10'000)); + RTC_CHECK(event.Wait(/*give_up_after=*/rtc::Event::kForever, + /*warn_after=*/TimeDelta::Seconds(10))); } class RTC_LOCKABLE TaskQueueForTest : public rtc::TaskQueue { diff --git a/test/time_controller/time_controller_conformance_test.cc b/test/time_controller/time_controller_conformance_test.cc index fa510b24df..b9417f4bb8 100644 --- a/test/time_controller/time_controller_conformance_test.cc +++ b/test/time_controller/time_controller_conformance_test.cc @@ -165,8 +165,7 @@ TEST_P(SimulatedRealTimeControllerConformanceTest, execution_order.Executed(2); event.Set(); }); - EXPECT_TRUE(event.Wait(/*give_up_after_ms=*/100, - /*warn_after_ms=*/10'000)); + EXPECT_TRUE(event.Wait(/*give_up_after_ms=*/100)); time_controller->AdvanceTime(TimeDelta::Millis(100)); EXPECT_THAT(execution_order.order(), ElementsAreArray({1, 2})); // Destroy `task_queue` before `execution_order` to be sure `execution_order`