Simplify thread yielding by removing support for high-priority tasks Remove the `PostHighPriorityTask` method and the internal `high_priority_messages_` queue from the `Thread` class. Instead, change `IsYieldRequested()` to return true whenever there are any pending tasks in the standard message queue, rather than only checking for high-priority tasks. This means that the sequence of batched operations is always executed when the thread is idle, rather than blocking the thread, ensuring that media-related calls to the same thread get processed as fast as possible. Bug: webrtc:42222804 Change-Id: I758c33dc2c33d79fc5e6beea1926529f78e49255 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/456760 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47249}
diff --git a/pc/scoped_operations_batcher.cc b/pc/scoped_operations_batcher.cc index 00c19cf..b5b277f 100644 --- a/pc/scoped_operations_batcher.cc +++ b/pc/scoped_operations_batcher.cc
@@ -55,7 +55,7 @@ } } ++task_idx; - if (!target_thread_is_current && target_thread_->IsYieldRequested()) { + if (!target_thread_is_current && target_thread_->HasPendingTasks()) { return; } }
diff --git a/pc/scoped_operations_batcher_unittest.cc b/pc/scoped_operations_batcher_unittest.cc index 0f188f7..03aa42d 100644 --- a/pc/scoped_operations_batcher_unittest.cc +++ b/pc/scoped_operations_batcher_unittest.cc
@@ -75,7 +75,7 @@ EXPECT_EQ(return_task_thread, signaling_thread); } -TEST(ScopedOperationsBatcherTest, YieldsToHighPriorityTasks) { +TEST(ScopedOperationsBatcherTest, YieldsToOtherTasks) { auto target_thread = Thread::Create(); target_thread->Start(); @@ -86,16 +86,17 @@ batcher.push_back([&] { execution_order.push_back(1); }); batcher.push_back([&] { execution_order.push_back(2); - // Post a high-priority task that should interrupt the batch. - target_thread->PostHighPriorityTask( - [&] { execution_order.push_back(3); }); + // Post a task that should interrupt the batch since we now yield to any + // pending task. + target_thread->PostTask([&] { execution_order.push_back(3); }); }); batcher.push_back([&] { execution_order.push_back(4); }); batcher.push_back([&] { execution_order.push_back(5); }); } - // Expect the high priority task (3) to execute immediately after the task - // that posted it (2). The regular tasks (4, 5) should yield. + // Expect the task (3) to execute immediately after the task + // that posted it (2). The operations remaining in the batch (4, 5) + // should resume after the thread has processed the new task. EXPECT_EQ(execution_order, std::vector<int>({1, 2, 3, 4, 5})); }
diff --git a/rtc_base/thread.cc b/rtc_base/thread.cc index 4bea295..81a7b6b 100644 --- a/rtc_base/thread.cc +++ b/rtc_base/thread.cc
@@ -391,7 +391,6 @@ ThreadManager::Remove(this); // Clear. CurrentTaskQueueSetter set_current(this); - high_priority_messages_.clear(); messages_.clear(); delayed_messages_ = {}; } @@ -442,13 +441,6 @@ messages_.push_back(std::move(delayed_messages_.top().functor)); delayed_messages_.pop(); } - // Pull a high priority message off the queue, if available. - if (!high_priority_messages_.empty()) { - absl::AnyInvocable<void() &&> task = - std::move(high_priority_messages_.front()); - high_priority_messages_.pop_front(); - return task; - } // Pull a message off the message queue, if available. if (!messages_.empty()) { absl::AnyInvocable<void() &&> task = std::move(messages_.front()); @@ -494,15 +486,6 @@ void Thread::PostTaskImpl(absl::AnyInvocable<void() &&> task, const PostTaskTraits& /* traits */, const Location& /* location */) { - PostTaskInternal(std::move(task), /*high_priority=*/false); -} - -void Thread::PostHighPriorityTask(absl::AnyInvocable<void() &&> task) { - PostTaskInternal(std::move(task), /*high_priority=*/true); -} - -void Thread::PostTaskInternal(absl::AnyInvocable<void() &&> task, - bool high_priority) { if (IsQuitting()) { return; } @@ -513,11 +496,7 @@ { MutexLock lock(&mutex_); - if (high_priority) { - high_priority_messages_.push_back(std::move(task)); - } else { - messages_.push_back(std::move(task)); - } + messages_.push_back(std::move(task)); WakeUpSocketServer(); } } @@ -554,7 +533,7 @@ int Thread::GetDelay() { MutexLock lock(&mutex_); - if (!high_priority_messages_.empty() || !messages_.empty()) + if (!messages_.empty()) return 0; if (!delayed_messages_.empty()) { @@ -990,7 +969,7 @@ } } -bool Thread::IsYieldRequested() const { +bool Thread::HasPendingTasks() const { RTC_DCHECK_RUN_ON(this); #if RTC_DCHECK_IS_ON // If you've hit this, then there's a cooperative task running from inside a @@ -998,7 +977,7 @@ RTC_DCHECK_EQ(running_synchronous_blocking_call_count_, 0); #endif MutexLock lock(&mutex_); - return !high_priority_messages_.empty(); + return !messages_.empty(); } } // namespace webrtc
diff --git a/rtc_base/thread.h b/rtc_base/thread.h index 0629461..2437272 100644 --- a/rtc_base/thread.h +++ b/rtc_base/thread.h
@@ -317,11 +317,12 @@ // ProcessMessages occasionally. virtual void Run(); - // Called from cooperative tasks to know if they should yield. + // Returns true if there are pending tasks in the message queue. + // Cooperative tasks can use this to know if they should yield. // If a task yields, it is up to the task itself how or if to // continue the ongoing operation. Typically this can be handled // by using PostTask() to queue up a continuation task. - bool IsYieldRequested() const; + bool HasPendingTasks() const; // Convenience method to invoke a functor on another thread. // Blocks the current thread until execution is complete. @@ -345,12 +346,6 @@ return result; } - // Posts a task that jumps the queue (gets added as the next task to execute, - // ahead of other pending tasks). An internal yielding request flag is also - // raised that cooperative aware tasks can check by calling - // `IsYieldRequested()` and immediately exit. - void PostHighPriorityTask(absl::AnyInvocable<void() &&> task); - // Allows BlockingCall to specified `thread`. Thread never will be // dereferenced and will be used only for reference-based comparison, so // instance can be safely deleted. If NDEBUG is defined and RTC_DCHECK_IS_ON @@ -476,8 +471,6 @@ private: static const int kSlowDispatchLoggingThreshold = 50; // 50 ms - void PostTaskInternal(absl::AnyInvocable<void() &&> task, bool high_priority); - // Get() will process I/O until: // 1) A task is available (returns it) // 2) cmsWait seconds have elapsed (returns empty task) @@ -513,8 +506,6 @@ void ClearCurrentTaskQueue(); std::deque<absl::AnyInvocable<void() &&>> messages_ RTC_GUARDED_BY(mutex_); - std::deque<absl::AnyInvocable<void() &&>> high_priority_messages_ - RTC_GUARDED_BY(mutex_); std::priority_queue<DelayedMessage> delayed_messages_ RTC_GUARDED_BY(mutex_); uint32_t delayed_next_num_ RTC_GUARDED_BY(mutex_); #if RTC_DCHECK_IS_ON
diff --git a/rtc_base/thread_unittest.cc b/rtc_base/thread_unittest.cc index 97ed808..ce011e7 100644 --- a/rtc_base/thread_unittest.cc +++ b/rtc_base/thread_unittest.cc
@@ -873,155 +873,54 @@ EXPECT_EQ(TaskQueueBase::Current(), current_tq); } -// Verifies that posting a high-priority task sets the "yield requested" flag -// on the target thread, allowing a cooperative task to observe it. -TEST(ThreadCooperativeTest, HighPriorityTaskTriggersYieldRequest) { +// Uses `HasPendingTasks()` to detect when to yield to another posted task. +TEST(ThreadCooperativeTest, TaskTriggersHasPendingTasks) { test::RunLoop loop; auto thread = Thread::Create(); thread->Start(); - bool yield_requested = false; + bool was_interrupted = false; Event task_started; - // Post a long running task that checks for yield + // Post a long running task that checks for pending tasks. thread->PostTask( - [&yield_requested, &loop, &task_started, thread = thread.get()] { + [&was_interrupted, &loop, &task_started, thread = thread.get()] { task_started.Set(); - while (!thread->IsYieldRequested()) { + while (!thread->HasPendingTasks()) { // Busy loop/simulated work } - loop.PostTask([&yield_requested, &loop] { - yield_requested = true; + loop.PostTask([&was_interrupted, &loop] { + was_interrupted = true; loop.Quit(); }); }); - // Wait for the task to start to ensure that the high priority task doesn't + // Wait for the task to start to ensure that the task doesn't // run first. task_started.Wait(Event::kForever); - // Post a high priority task that interrupts the busy loop. - thread->PostHighPriorityTask([] {}); + // Post a task that interrupts the busy loop. + thread->PostTask([] {}); loop.Run(); - EXPECT_TRUE(yield_requested); + EXPECT_TRUE(was_interrupted); } -// Verifies that a high-priority task is executed before a regular priority task -// that was posted earlier, if the thread was busy. -TEST(ThreadCooperativeTest, HighPriorityTaskRunsFirst) { - test::RunLoop loop; - auto thread = Thread::Create(); - thread->Start(); - - std::vector<std::string> execution_order; - Event task1_started; - Event continue_execution; - - // Task 1: Blocks the thread until signaled. - thread->PostTask([&] { - task1_started.Set(); - continue_execution.Wait(Event::kForever); - loop.PostTask([&] { execution_order.push_back("Task1"); }); - }); - - task1_started.Wait(Event::kForever); - - // Task 2: Regular priority. - thread->PostTask( - [&] { loop.PostTask([&] { execution_order.push_back("Task2"); }); }); - - // Task 3: High priority. High priority tasks should run before "Task2". - thread->PostHighPriorityTask( - [&] { loop.PostTask([&] { execution_order.push_back("Task3"); }); }); - - // Signal Task 1 to continue. - continue_execution.Set(); - - // Now post a task to quit the loop. - thread->PostTask([&] { loop.PostTask([&] { loop.Quit(); }); }); - - loop.Run(); - - // Expect Task1 (resuming), Task3 (High Prio), Task2 (Low Prio - pushed to - // back). - EXPECT_THAT(execution_order, ElementsAre("Task1", "Task3", "Task2")); -} - -// Verifies that multiple high-priority tasks execute in the sequence they were -// posted (FIFO order). -TEST(ThreadCooperativeTest, HighPriorityTasksExecuteInSequence) { - test::RunLoop loop; - auto thread = Thread::Create(); - thread->Start(); - - std::vector<std::string> execution_order; - Event task1_started; - Event continue_execution; - - // Block the thread. - thread->PostTask([&] { - task1_started.Set(); - continue_execution.Wait(Event::kForever); - }); - - task1_started.Wait(Event::kForever); - - // Post a normal priority task first to ensure high priority tasks run before - // it. - thread->PostTask( - [&] { loop.PostTask([&] { execution_order.push_back("Normal1"); }); }); - - // Post multiple high priority tasks while the thread is blocked. - thread->PostHighPriorityTask( - [&] { loop.PostTask([&] { execution_order.push_back("HP1"); }); }); - thread->PostHighPriorityTask( - [&] { loop.PostTask([&] { execution_order.push_back("HP2"); }); }); - thread->PostHighPriorityTask( - [&] { loop.PostTask([&] { execution_order.push_back("HP3"); }); }); - - // Unblock the thread. - continue_execution.Set(); - - // Quit after all tasks. - thread->PostTask([&] { loop.PostTask([&] { loop.Quit(); }); }); - - loop.Run(); - - EXPECT_THAT(execution_order, ElementsAre("HP1", "HP2", "HP3", "Normal1")); -} - -TEST(ThreadCooperativeTest, YieldRequestedClearedAfterHighPriorityTask) { +TEST(ThreadCooperativeTest, HasPendingTasksClearedAfterTask) { std::unique_ptr<Thread> thread(Thread::Create()); thread->Start(); // Initially false. - thread->BlockingCall([&] { EXPECT_FALSE(thread->IsYieldRequested()); }); + thread->BlockingCall([&] { EXPECT_FALSE(thread->HasPendingTasks()); }); - // Post high priority task. - thread->PostHighPriorityTask([&] {}); + // Post task. + thread->PostTask([&] {}); - // Post normal task. - thread->BlockingCall([&] { EXPECT_FALSE(thread->IsYieldRequested()); }); + // Use `BlockingCall` to post normal task which implicitly blocks and waits + // for its functor to run, at which point the queue will be empty again. + thread->BlockingCall([&] { EXPECT_FALSE(thread->HasPendingTasks()); }); } -#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) && RTC_DCHECK_IS_ON -TEST(ThreadCooperativeDeathTest, YieldInSynchronousBlockingCallTriggersDeath) { - std::unique_ptr<Thread> background_thread = Thread::Create(); - background_thread->Start(); - background_thread->BlockingCall([&] { - // Yielding from a top-level synchronous blocking call is allowed. - background_thread->IsYieldRequested(); - - // Call a nested BlockingCall. - background_thread->BlockingCall([&] { - // This violates the "no yielding in blocking call" rule. - EXPECT_DEATH(background_thread->IsYieldRequested(), ""); - }); - }); -} -#endif - class ThreadFactory : public TaskQueueFactory { public: std::unique_ptr<TaskQueueBase, TaskQueueDeleter> CreateTaskQueue(