Skip to content

[libc++] Refactor atomic_wait using lambdas #115746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Nov 11, 2024

Now that we've dropped support for older C++ dialects in the synchronization library, we can use lambdas to clarify some of the code used to implement atomic_wait.

@ldionne ldionne requested a review from a team as a code owner November 11, 2024 17:15
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 11, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Now that we've dropped support for older C++ dialects in the synchronization library, we can use lambdas to clarify some of the code used to implement atomic_wait.


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

3 Files Affected:

  • (modified) libcxx/include/__atomic/atomic_sync.h (+13-30)
  • (modified) libcxx/include/latch (+3-2)
  • (modified) libcxx/include/semaphore (+3-2)
diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h
index 08f3497fef9f45..2b929d8e8775dc 100644
--- a/libcxx/include/__atomic/atomic_sync.h
+++ b/libcxx/include/__atomic/atomic_sync.h
@@ -57,18 +57,6 @@ struct __atomic_waitable< _Tp,
                                    decltype(__atomic_waitable_traits<__decay_t<_Tp> >::__atomic_contention_address(
                                        std::declval<const _Tp&>()))> > : true_type {};
 
-template <class _AtomicWaitable, class _Poll>
-struct __atomic_wait_poll_impl {
-  const _AtomicWaitable& __a_;
-  _Poll __poll_;
-  memory_order __order_;
-
-  _LIBCPP_HIDE_FROM_ABI bool operator()() const {
-    auto __current_val = __atomic_waitable_traits<__decay_t<_AtomicWaitable> >::__atomic_load(__a_, __order_);
-    return __poll_(__current_val);
-  }
-};
-
 #if _LIBCPP_HAS_THREADS
 
 _LIBCPP_AVAILABILITY_SYNC _LIBCPP_EXPORTED_FROM_ABI void __cxx_atomic_notify_one(void const volatile*) _NOEXCEPT;
@@ -144,11 +132,12 @@ struct __atomic_wait_backoff_impl {
 // value. The predicate function must not return `false` spuriously.
 template <class _AtomicWaitable, class _Poll>
 _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
-__atomic_wait_unless(const _AtomicWaitable& __a, _Poll&& __poll, memory_order __order) {
+__atomic_wait_unless(const _AtomicWaitable& __a, memory_order __order, _Poll&& __poll) {
   static_assert(__atomic_waitable<_AtomicWaitable>::value, "");
-  __atomic_wait_poll_impl<_AtomicWaitable, __decay_t<_Poll> > __poll_impl     = {__a, __poll, __order};
   __atomic_wait_backoff_impl<_AtomicWaitable, __decay_t<_Poll> > __backoff_fn = {__a, __poll, __order};
-  std::__libcpp_thread_poll_with_backoff(__poll_impl, __backoff_fn);
+  std::__libcpp_thread_poll_with_backoff(
+      /* poll */ [&]() { return __poll(__atomic_waitable_traits<_AtomicWaitable>::__atomic_load(__a, __order)); },
+      /* backoff */ __backoff_fn);
 }
 
 template <class _AtomicWaitable>
@@ -166,9 +155,10 @@ _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void __atomic_notify_all(const _
 #else // _LIBCPP_HAS_THREADS
 
 template <class _AtomicWaitable, class _Poll>
-_LIBCPP_HIDE_FROM_ABI void __atomic_wait_unless(const _AtomicWaitable& __a, _Poll&& __poll, memory_order __order) {
-  __atomic_wait_poll_impl<_AtomicWaitable, __decay_t<_Poll> > __poll_fn = {__a, __poll, __order};
-  std::__libcpp_thread_poll_with_backoff(__poll_fn, __spinning_backoff_policy());
+_LIBCPP_HIDE_FROM_ABI void __atomic_wait_unless(const _AtomicWaitable& __a, memory_order __order, _Poll&& __poll) {
+  std::__libcpp_thread_poll_with_backoff(
+      /* poll */ [&]() { return __poll(__atomic_waitable_traits<_AtomicWaitable>::__atomic_load(__a, __order)); },
+      /* backoff */ __spinning_backoff_policy());
 }
 
 template <class _AtomicWaitable>
@@ -184,20 +174,13 @@ _LIBCPP_HIDE_FROM_ABI bool __cxx_nonatomic_compare_equal(_Tp const& __lhs, _Tp c
   return std::memcmp(std::addressof(__lhs), std::addressof(__rhs), sizeof(_Tp)) == 0;
 }
 
-template <class _Tp>
-struct __atomic_compare_unequal_to {
-  _Tp __val_;
-  _LIBCPP_HIDE_FROM_ABI bool operator()(const _Tp& __arg) const {
-    return !std::__cxx_nonatomic_compare_equal(__arg, __val_);
-  }
-};
-
-template <class _AtomicWaitable, class _Up>
+template <class _AtomicWaitable, class _Tp>
 _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
-__atomic_wait(_AtomicWaitable& __a, _Up __val, memory_order __order) {
+__atomic_wait(_AtomicWaitable& __a, _Tp __val, memory_order __order) {
   static_assert(__atomic_waitable<_AtomicWaitable>::value, "");
-  __atomic_compare_unequal_to<_Up> __nonatomic_equal = {__val};
-  std::__atomic_wait_unless(__a, __nonatomic_equal, __order);
+  std::__atomic_wait_unless(__a, __order, [&](_Tp const& __current) {
+    return !std::__cxx_nonatomic_compare_equal(__current, __val)
+  });
 }
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/latch b/libcxx/include/latch
index 90cca27c50c376..5de59a37e4deec 100644
--- a/libcxx/include/latch
+++ b/libcxx/include/latch
@@ -99,8 +99,9 @@ public:
     return try_wait_impl(__value);
   }
   inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void wait() const {
-    std::__atomic_wait_unless(
-        __a_, [this](ptrdiff_t& __value) -> bool { return try_wait_impl(__value); }, memory_order_acquire);
+    std::__atomic_wait_unless(__a_, memory_order_acquire, [this](ptrdiff_t& __value) -> bool {
+      return try_wait_impl(__value);
+    });
   }
   inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void arrive_and_wait(ptrdiff_t __update = 1) {
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update >= 0, "latch::arrive_and_wait called with a negative value");
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index 05c85bc810603e..c4b4028fad7c61 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -96,8 +96,9 @@ public:
     }
   }
   _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
-    std::__atomic_wait_unless(
-        __a_, [this](ptrdiff_t& __old) { return __try_acquire_impl(__old); }, memory_order_relaxed);
+    std::__atomic_wait_unless(__a_, memory_order_relaxed, [this](ptrdiff_t& __old) {
+      return __try_acquire_impl(__old);
+    });
   }
   template <class _Rep, class _Period>
   _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool

Copy link
Member

@huixie90 huixie90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldionne ldionne force-pushed the review/refactor-atomic_wait branch from f07d572 to 293a173 Compare November 11, 2024 18:03
@ldionne ldionne force-pushed the review/refactor-atomic_wait branch from 293a173 to f781947 Compare November 19, 2024 23:36
Now that we've dropped support for older C++ dialects in the
synchronization library, we can use lambdas to clarify some of
the code used to implement atomic_wait.
@ldionne ldionne force-pushed the review/refactor-atomic_wait branch from f781947 to 3bad740 Compare November 22, 2024 08:44
@ldionne ldionne merged commit d681e10 into llvm:main Nov 27, 2024
66 checks passed
@ldionne ldionne deleted the review/refactor-atomic_wait branch November 27, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants