[libc++] Add an implementation of shared_mutex based on pthread_rwlock_t #148046
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mutex Before this patch, we implemented both shared_mutex and shared_timed_mutex using two condition variables and a regular mutex. We did so by using the __shared_mutex_base class from both shared_mutex and shared_timed_mutex, and having __shared_mutex_base implement this logic. However, it may be desirable to use a different implementation for shared_mutex and shared_timed_mutex, since a regular shared_mutex could conceivably use pthread_rwlock_t instead for better performance. This refactoring opens the door to making such a change (under an ABI macro of course).
This patch introduces a new implementation of shared_mutex based on pthread_rwlock_t instead of two condition variables and a conventional mutex. It also adds a new optional component to the base thread API to represent a native read/write lock. This must be introduced behind an ABI flag because it requires changing the representation of std::shared_mutex, which is an ABI break. Note that if __shared_mutex_base had been implemented fully inside the dylib, we would have been able to take this change without breaking the ABI. We would have done this by ensuring that __shared_mutex_base retains the same size and alignment as before and just switch the implementation to use pthread_rwlock_t. Sadly, __shared_mutex_base's destructor is defined in the headers as `= default`, which means that code out there may exist with an inlined definition for that destructor. Hence, changing the semantics of the members of the class would break the assumptions baked into that inlined code. I also explored the possibility of first moving ~__shared_mutex_base to the dylib using an availability macro, and then trying to implement this optimization in a backwards compatible manner. Sadly, there is no way around the fact that inlined code for ~__shared_mutex_base may exist out there, so we'd have to wait until we can assume the absence of any such code, and there's no horizon for that. And even then, making this change in an non-ABI breaking way would require artificially keeping sizeof(__shared_mutex_base) stable when it could be smaller. Moving ~__shared_mutex_base to the dylib is something that we could pursue separately with its own merits, however I didn't want to couple it to this optimization since it didn't seem worth it.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesNOTE: This PR is based on top of #148044 This patch introduces a new implementation of shared_mutex based on This must be introduced behind an ABI flag because it requires changing I also explored the possibility of first moving ~__shared_mutex_base to Moving ~__shared_mutex_base to the dylib is something that we could Full diff: https://github.com/llvm/llvm-project/pull/148046.diff 8 Files Affected:
diff --git a/libcxx/docs/ABIGuarantees.rst b/libcxx/docs/ABIGuarantees.rst
index c7d5afe1080bb..3020a8ce855d2 100644
--- a/libcxx/docs/ABIGuarantees.rst
+++ b/libcxx/docs/ABIGuarantees.rst
@@ -80,6 +80,11 @@ flag removes that artificial padding.
``char_traits<char_type>::eq_int_type()`` cannot distinguish between ``WEOF`` and ``WCHAR_MAX``. This flag changes
``basic_ios`` to instead track whether the fill value has been initialized using a separate boolean.
+``_LIBCPP_ABI_USE_NATIVE_SHARED_MUTEX``
+---------------------------------------
+This macro switches ``std::shared_mutex`` to use a native read/write lock if one is provided by the system. This is an
+ABI break because it changes the representation of ``std::shared_mutex`` in a backwards incompatible way.
+
Linking TUs which have been compiled against different releases of libc++
=========================================================================
diff --git a/libcxx/include/__configuration/abi.h b/libcxx/include/__configuration/abi.h
index a75cd0a675339..fa98ed0d4138f 100644
--- a/libcxx/include/__configuration/abi.h
+++ b/libcxx/include/__configuration/abi.h
@@ -78,6 +78,7 @@
# define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
# define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
# define _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+# define _LIBCPP_ABI_USE_NATIVE_SHARED_MUTEX
#elif _LIBCPP_ABI_VERSION == 1
# if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
diff --git a/libcxx/include/__thread/support.h b/libcxx/include/__thread/support.h
index 50a18daf2b685..1a996012b1cdd 100644
--- a/libcxx/include/__thread/support.h
+++ b/libcxx/include/__thread/support.h
@@ -46,6 +46,20 @@ _LIBCPP_NO_THREAD_SAFETY_ANALYSIS bool __libcpp_mutex_trylock(__libcpp_mutex_t*)
_LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_mutex_unlock(__libcpp_mutex_t*);
int __libcpp_mutex_destroy(__libcpp_mutex_t*);
+//
+// Shared mutex (optional API)
+//
+#define _LIBCPP_HAS_NATIVE_RW_MUTEX {0|1}
+using __libcpp_rw_mutex_t = ...;
+#define _LIBCPP_RW_MUTEX_INITIALIZER ...
+
+_LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_rw_mutex_lock(__libcpp_rw_mutex_t*);
+_LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_rw_mutex_lock_shared(__libcpp_rw_mutex_t*);
+_LIBCPP_NO_THREAD_SAFETY_ANALYSIS bool __libcpp_rw_mutex_trylock(__libcpp_rw_mutex_t*);
+_LIBCPP_NO_THREAD_SAFETY_ANALYSIS bool __libcpp_rw_mutex_trylock_shared(__libcpp_rw_mutex_t*);
+_LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_rw_mutex_unlock(__libcpp_rw_mutex_t*);
+int __libcpp_rw_mutex_destroy(__libcpp_rw_mutex_t*);
+
//
// Condition Variable
//
diff --git a/libcxx/include/__thread/support/c11.h b/libcxx/include/__thread/support/c11.h
index fe00a2d97fadc..7e51f12fb8091 100644
--- a/libcxx/include/__thread/support/c11.h
+++ b/libcxx/include/__thread/support/c11.h
@@ -76,6 +76,11 @@ inline _LIBCPP_HIDE_FROM_ABI int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) {
return 0;
}
+//
+// Shared mutex (C11 doesn't provide one)
+//
+#define _LIBCPP_HAS_NATIVE_RW_MUTEX 0
+
//
// Condition Variable
//
diff --git a/libcxx/include/__thread/support/pthread.h b/libcxx/include/__thread/support/pthread.h
index 14e92079dadfe..0fc7c410aa31b 100644
--- a/libcxx/include/__thread/support/pthread.h
+++ b/libcxx/include/__thread/support/pthread.h
@@ -105,6 +105,40 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_mute
inline _LIBCPP_HIDE_FROM_ABI int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) { return pthread_mutex_destroy(__m); }
+//
+// Shared mutex
+//
+#define _LIBCPP_HAS_NATIVE_RW_MUTEX 1
+using __libcpp_rw_mutex_t = pthread_rwlock_t;
+#define _LIBCPP_RW_MUTEX_INITIALIZER PTHREAD_RWLOCK_INITIALIZER
+
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_rw_mutex_lock(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_wrlock(__m);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS int
+__libcpp_rw_mutex_lock_shared(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_rdlock(__m);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS bool
+__libcpp_rw_mutex_trylock(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_trywrlock(__m) == 0;
+}
+
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS bool
+__libcpp_rw_mutex_trylock_shared(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_tryrdlock(__m) == 0;
+}
+
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_rw_mutex_unlock(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_unlock(__m);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI int __libcpp_rw_mutex_destroy(__libcpp_rw_mutex_t* __m) {
+ return pthread_rwlock_destroy(__m);
+}
+
//
// Condition Variable
//
diff --git a/libcxx/include/__thread/support/windows.h b/libcxx/include/__thread/support/windows.h
index 5dc4fa14f45b6..6d340ce604540 100644
--- a/libcxx/include/__thread/support/windows.h
+++ b/libcxx/include/__thread/support/windows.h
@@ -57,6 +57,11 @@ _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_NO_THREAD_SAFETY_ANALYSIS int __libcpp_mutex_u
_LIBCPP_EXPORTED_FROM_ABI int __libcpp_mutex_destroy(__libcpp_mutex_t* __m);
+//
+// Shared mutex (TODO: Does Windows provide a native read/write mutex?)
+//
+#define _LIBCPP_HAS_NATIVE_RW_MUTEX 0
+
//
// Condition variable
//
diff --git a/libcxx/include/shared_mutex b/libcxx/include/shared_mutex
index 8c02e348e4de7..451dfa1459fde 100644
--- a/libcxx/include/shared_mutex
+++ b/libcxx/include/shared_mutex
@@ -129,6 +129,7 @@ template <class Mutex>
# if _LIBCPP_HAS_THREADS
+# include <__assert>
# include <__chrono/duration.h>
# include <__chrono/steady_clock.h>
# include <__chrono/time_point.h>
@@ -138,6 +139,7 @@ template <class Mutex>
# include <__mutex/tag_types.h>
# include <__mutex/unique_lock.h>
# include <__system_error/throw_system_error.h>
+# include <__thread/support.h>
# include <__utility/swap.h>
# include <cerrno>
# include <version>
@@ -153,7 +155,34 @@ _LIBCPP_PUSH_MACROS
_LIBCPP_BEGIN_NAMESPACE_STD
-struct _LIBCPP_EXPORTED_FROM_ABI __shared_mutex_base {
+# if _LIBCPP_HAS_NATIVE_RW_MUTEX
+class __native_shared_mutex {
+ __libcpp_rw_mutex_t __mut_;
+
+public:
+ _LIBCPP_HIDE_FROM_ABI __native_shared_mutex() : __mut_(_LIBCPP_RW_MUTEX_INITIALIZER) {}
+ _LIBCPP_HIDE_FROM_ABI ~__native_shared_mutex() {
+ int __ec = std::__libcpp_rw_mutex_destroy(&__mut_);
+ (void)__ec;
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(__ec == 0, "destroying a read/write mutex failed");
+ }
+
+ __native_shared_mutex(const __native_shared_mutex&) = delete;
+ __native_shared_mutex& operator=(const __native_shared_mutex&) = delete;
+
+ // Exclusive ownership
+ _LIBCPP_HIDE_FROM_ABI void lock();
+ _LIBCPP_HIDE_FROM_ABI bool try_lock();
+ _LIBCPP_HIDE_FROM_ABI void unlock();
+
+ // Shared ownership
+ _LIBCPP_HIDE_FROM_ABI void lock_shared();
+ _LIBCPP_HIDE_FROM_ABI bool try_lock_shared();
+ _LIBCPP_HIDE_FROM_ABI void unlock_shared();
+};
+# endif // _LIBCPP_HAS_NATIVE_RW_MUTEX
+
+struct __dual_cv_shared_mutex {
mutex __mut_;
condition_variable __gate1_;
condition_variable __gate2_;
@@ -162,6 +191,26 @@ struct _LIBCPP_EXPORTED_FROM_ABI __shared_mutex_base {
static const unsigned __write_entered_ = 1U << (sizeof(unsigned) * __CHAR_BIT__ - 1);
static const unsigned __n_readers_ = ~__write_entered_;
+ _LIBCPP_HIDE_FROM_ABI __dual_cv_shared_mutex() : __state_(0) {}
+ _LIBCPP_HIDE_FROM_ABI ~__dual_cv_shared_mutex() = default;
+
+ __dual_cv_shared_mutex(const __dual_cv_shared_mutex&) = delete;
+ __dual_cv_shared_mutex& operator=(const __dual_cv_shared_mutex&) = delete;
+
+ // Exclusive ownership
+ _LIBCPP_HIDE_FROM_ABI void lock();
+ _LIBCPP_HIDE_FROM_ABI bool try_lock();
+ _LIBCPP_HIDE_FROM_ABI void unlock();
+
+ // Shared ownership
+ _LIBCPP_HIDE_FROM_ABI void lock_shared();
+ _LIBCPP_HIDE_FROM_ABI bool try_lock_shared();
+ _LIBCPP_HIDE_FROM_ABI void unlock_shared();
+};
+
+struct _LIBCPP_EXPORTED_FROM_ABI __shared_mutex_base {
+ __dual_cv_shared_mutex __impl_;
+
__shared_mutex_base();
_LIBCPP_HIDE_FROM_ABI ~__shared_mutex_base() = default;
@@ -184,7 +233,11 @@ struct _LIBCPP_EXPORTED_FROM_ABI __shared_mutex_base {
# if _LIBCPP_STD_VER >= 17
class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_CAPABILITY("shared_mutex") shared_mutex {
+# if defined(_LIBCPP_ABI_USE_NATIVE_SHARED_MUTEX) && _LIBCPP_HAS_NATIVE_RW_MUTEX
+ __native_shared_mutex __base_;
+# else
__shared_mutex_base __base_;
+# endif
public:
_LIBCPP_HIDE_FROM_ABI shared_mutex() : __base_() {}
@@ -211,7 +264,7 @@ public:
# endif
class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_CAPABILITY("shared_timed_mutex") shared_timed_mutex {
- __shared_mutex_base __base_;
+ __dual_cv_shared_mutex __base_;
public:
shared_timed_mutex();
diff --git a/libcxx/src/shared_mutex.cpp b/libcxx/src/shared_mutex.cpp
index 6180833736956..f4de9ae176187 100644
--- a/libcxx/src/shared_mutex.cpp
+++ b/libcxx/src/shared_mutex.cpp
@@ -14,12 +14,11 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-// Shared Mutex Base
-__shared_mutex_base::__shared_mutex_base() : __state_(0) {}
-
-// Exclusive ownership
-
-void __shared_mutex_base::lock() {
+// Dual condition_variable implementaiton of shared_mutex
+//
+// These methods are defined here because they're only called from the dylib,
+// but they are not part of the library's ABI.
+_LIBCPP_HIDE_FROM_ABI void __dual_cv_shared_mutex::lock() {
unique_lock<mutex> lk(__mut_);
while (__state_ & __write_entered_)
__gate1_.wait(lk);
@@ -28,7 +27,7 @@ void __shared_mutex_base::lock() {
__gate2_.wait(lk);
}
-bool __shared_mutex_base::try_lock() {
+_LIBCPP_HIDE_FROM_ABI bool __dual_cv_shared_mutex::try_lock() {
unique_lock<mutex> lk(__mut_);
if (__state_ == 0) {
__state_ = __write_entered_;
@@ -37,7 +36,7 @@ bool __shared_mutex_base::try_lock() {
return false;
}
-void __shared_mutex_base::unlock() {
+_LIBCPP_HIDE_FROM_ABI void __dual_cv_shared_mutex::unlock() {
{
lock_guard<mutex> _(__mut_);
__state_ = 0;
@@ -45,9 +44,7 @@ void __shared_mutex_base::unlock() {
__gate1_.notify_all();
}
-// Shared ownership
-
-void __shared_mutex_base::lock_shared() {
+_LIBCPP_HIDE_FROM_ABI void __dual_cv_shared_mutex::lock_shared() {
unique_lock<mutex> lk(__mut_);
while ((__state_ & __write_entered_) || (__state_ & __n_readers_) == __n_readers_)
__gate1_.wait(lk);
@@ -56,7 +53,7 @@ void __shared_mutex_base::lock_shared() {
__state_ |= num_readers;
}
-bool __shared_mutex_base::try_lock_shared() {
+_LIBCPP_HIDE_FROM_ABI bool __dual_cv_shared_mutex::try_lock_shared() {
unique_lock<mutex> lk(__mut_);
unsigned num_readers = __state_ & __n_readers_;
if (!(__state_ & __write_entered_) && num_readers != __n_readers_) {
@@ -68,7 +65,7 @@ bool __shared_mutex_base::try_lock_shared() {
return false;
}
-void __shared_mutex_base::unlock_shared() {
+_LIBCPP_HIDE_FROM_ABI void __dual_cv_shared_mutex::unlock_shared() {
unique_lock<mutex> lk(__mut_);
unsigned num_readers = (__state_ & __n_readers_) - 1;
__state_ &= ~__n_readers_;
@@ -86,6 +83,62 @@ void __shared_mutex_base::unlock_shared() {
}
}
+// Native implementation of shared_mutex
+//
+// Similarly, those are not part of the dylib's ABI but they are defined here because they
+// are only used from within the dylib.
+#if _LIBCPP_HAS_NATIVE_RW_MUTEX
+_LIBCPP_HIDE_FROM_ABI void __native_shared_mutex::lock() {
+ int __ec = std::__libcpp_rw_mutex_lock(&__mut_);
+ if (__ec != 0)
+ std::__throw_system_error(__ec, "shared_mutex::lock failed");
+}
+
+_LIBCPP_HIDE_FROM_ABI bool __native_shared_mutex::try_lock() { return std::__libcpp_rw_mutex_trylock(&__mut_); }
+
+_LIBCPP_HIDE_FROM_ABI void __native_shared_mutex::unlock() {
+ int __ec = std::__libcpp_rw_mutex_unlock(&__mut_);
+ (void)__ec;
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(
+ __ec == 0, "call to shared_mutex::unlock failed. A possible reason is that the mutex wasn't locked");
+}
+
+// Shared ownership
+_LIBCPP_HIDE_FROM_ABI void __native_shared_mutex::lock_shared() {
+ int __ec = std::__libcpp_rw_mutex_lock_shared(&__mut_);
+ if (__ec != 0)
+ std::__throw_system_error(__ec, "shared_mutex::lock_shared failed");
+}
+
+_LIBCPP_HIDE_FROM_ABI bool __native_shared_mutex::try_lock_shared() {
+ return std::__libcpp_rw_mutex_trylock_shared(&__mut_);
+}
+
+_LIBCPP_HIDE_FROM_ABI void __native_shared_mutex::unlock_shared() {
+ int __ec = std::__libcpp_rw_mutex_unlock(&__mut_);
+ (void)__ec;
+ _LIBCPP_ASSERT_VALID_EXTERNAL_API_CALL(
+ __ec == 0, "call to shared_mutex::unlock_shared failed. A possible reason is that the mutex wasn't locked");
+}
+#endif // _LIBCPP_HAS_NATIVE_RW_MUTEX
+
+// Shared Mutex Base
+__shared_mutex_base::__shared_mutex_base() {}
+
+// Exclusive ownership
+void __shared_mutex_base::lock() { __impl_.lock(); }
+
+bool __shared_mutex_base::try_lock() { return __impl_.try_lock(); }
+
+void __shared_mutex_base::unlock() { __impl_.unlock(); }
+
+// Shared ownership
+void __shared_mutex_base::lock_shared() { __impl_.lock_shared(); }
+
+bool __shared_mutex_base::try_lock_shared() { return __impl_.try_lock_shared(); }
+
+void __shared_mutex_base::unlock_shared() { __impl_.unlock_shared(); }
+
// Shared Timed Mutex
// These routines are here for ABI stability
shared_timed_mutex::shared_timed_mutex() : __base_() {}
|
|
Moving the destructor to the dylib is something we could do if we wanted, #148047 implements that change. |
|
I also just discovered the existence of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This PR is based on top of #148044
This patch introduces a new implementation of shared_mutex based on
pthread_rwlock_t instead of two condition variables and a conventional
mutex. It also adds a new optional component to the base thread API to
represent a native read/write lock.
This must be introduced behind an ABI flag because it requires changing
the representation of std::shared_mutex, which is an ABI break. Note
that if __shared_mutex_base had been implemented fully inside the dylib,
we would have been able to take this change without breaking the ABI.
We would have done this by ensuring that __shared_mutex_base retains
the same size and alignment as before and just switch the implementation
to use pthread_rwlock_t. Sadly, __shared_mutex_base's destructor is
defined in the headers as
= default, which means that code out theremay exist with an inlined definition for that destructor. Hence, changing
the semantics of the members of the class would break the assumptions
baked into that inlined code.
I also explored the possibility of first moving ~__shared_mutex_base to
the dylib using an availability macro, and then trying to implement this
optimization in a backwards compatible manner. Sadly, there is no way
around the fact that inlined code for ~__shared_mutex_base may exist
out there, so we'd have to wait until we can assume the absence of any
such code, and there's no horizon for that. And even then, making this
change in an non-ABI breaking way would require artificially keeping
sizeof(__shared_mutex_base) stable when it could be smaller.
Moving ~__shared_mutex_base to the dylib is something that we could
pursue separately with its own merits, however I didn't want to couple
it to this optimization since it didn't seem worth it.