[libc++] Explicitly use the dual-cv implementation from shared_timed_mutex

[ldionne]

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).

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

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).

---

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

2 Files Affected:

• (modified) libcxx/include/shared_mutex (+22-2)
• (modified) libcxx/src/shared_mutex.cpp (+27-13)

diff --git a/libcxx/include/shared_mutex b/libcxx/include/shared_mutex
index 8c02e348e4de7..6100b6ce5e62e 100644
--- a/libcxx/include/shared_mutex
+++ b/libcxx/include/shared_mutex
@@ -153,7 +153,7 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-struct _LIBCPP_EXPORTED_FROM_ABI __shared_mutex_base {
+struct __dual_cv_shared_mutex {
   mutex __mut_;
   condition_variable __gate1_;
   condition_variable __gate2_;
@@ -162,6 +162,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;
 
@@ -211,7 +231,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..cc74ba9fee771 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,23 @@ void __shared_mutex_base::unlock_shared() {
   }
 }
 
+// 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_() {}

[philnik777]

Adding _LIBCPP_HIDE_FROM_ABI to something that is implemented in the dylib like this feels very wrong. I also don't understand why we need to intoduce __dual_cv_shared_mutex. This seems like more effort than it's worth. We can just introduce a new __native_shared_mutex and keep the old __shared_mutex_base as the __dual_cv version. The naming might not be perfect, but we have to keep it around anyways, so I don't see how this improves anything.