[LucasChollet]

Fixes #207763
On single-core or heavily loaded systems, using this_thread::yield()
prevents the notifier from burning all its allocated time on the
busy-loop.

The measurements were done on my Linux box, the time is the one reported
by lit. To isolate the test on a single core I used the following
commands:

export LIT_FILTER="wait/lost_wakeup.pass.cpp"
taskset -c 0 ninja -C build check-cxx

Configuration	Linux futex		Fallback
	Normal	Pinned core	Normal	Pinned core
Before	1.50s	8.44s	1.57s	301.02s
With yield	1.73s	7.36s	1.64s	8.88s

Both configurations are with the run < 10 change.

[llvmorg-github-actions]

@llvm/pr-subscribers-libcxx

Author: Lucas Chollet (LucasChollet)

Changes

Using __libcpp_thread_poll_with_backoff brings two improvements. First, in case of a lost wake-up, we get an assertion failure instead of a hang. And second, on single-core or heavily loaded systems, the notifier yield instead of burning all its allocated time on the busy-loop.

The measurements were done on my Linux box, the time is the one reported by lit. To isolate the test on a single core I used the following commands:

export LIT_FILTER="wait/lost_wakeup.pass.cpp"
taskset -c 0 ninja -C build check-cxx

Configuration	Linux futex		Fallback
	Normal	Pinned core	Normal	Pinned core
Before	1.20s	15.54s	1.26s	601.25s
With backoff	1.11s	14.42s	1.15s	17.60s

---

Full diff: https://github.com/llvm/llvm-project/pull/211242.diff

1 Files Affected:

• (modified) libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/lost_wakeup.pass.cpp (+6-2)

diff --git a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/lost_wakeup.pass.cpp b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/lost_wakeup.pass.cpp
index 17ad72dd74866..6b486173a4743 100644
--- a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/lost_wakeup.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/lost_wakeup.pass.cpp
@@ -14,6 +14,8 @@
 // <atomic>
 
 #include <atomic>
+#include <cassert>
+#include <chrono>
 #include <functional>
 #include <thread>
 #include <vector>
@@ -38,8 +40,10 @@ int main(int, char**) {
 
     auto notify = [&] {
       for (int i = 0; i < num_iterations; ++i) {
-        while (waiter_ready.load() < num_waiters) {
-        }
+        assert(std::__libcpp_thread_poll_with_backoff(
+                   [&]() -> bool { return waiter_ready.load() == num_waiters; },
+                   std::__libcpp_timed_backoff_policy(),
+                   std::chrono::seconds(1)) == std::__poll_with_backoff_results::__poll_success);
         waiter_ready.store(0);
         state.fetch_add(1);
         state.notify_all();

[huixie90]

After discussion offline, I think my opinion is that we should avoid using __libcpp_thread_poll_with_backoff in the test, because

1. It is an internal implementation detail function
2. It is a function that implements atomic::wait, and this test is exercising atomic::wait. Using its implementation detail to test itself is not a good idea.

I wonder if we could just use yield (excluding macOS) to see if this help with the test. something like this
#211261

[LucasChollet]

Updated this PR to follow your suggestion. I also measured the execution time again and updated the table.

[philnik777]

What's this about?

[LucasChollet]

From #161086 (comment):

I remember that macOS would put the yield thread on the lowest priority on the scheduler. If the machine was overloaded with other running jobs, our notifier thread would have taken long time to resume again.

[LucasChollet]

@philnik777 @huixie90 This is ready for review.

[philnik777]

I'd really like @huixie90 to take a look at this, since I'm not particularly familiar with this part of the code base.

[huixie90]

Given the comment here suggested that this change does improve testing time on some platforms #211261 (comment). LGTM

[LucasChollet]

Thanks for the approval! I don't have merge permissions though, could you please press the merge button for me?