Add writer priority mode in TimedRWMutex

Summary:
- The current TimedRwMutex behavior is biased heavily towards readers. This can lead to writer starvation under heavy read load which can result in writers unable to acquire the lock for several minutes
- The current change adds a writer priority mode to TimedRWMutex which can be specified at creation time. The default behavior will still be reader priority and since the logic is handled through templates it shouldn't introduce any side effects for the reader priority behavior

Reviewed By: matheweis

Differential Revision: D15204620

fbshipit-source-id: e940868aa9cec8f4a0a6b59c695ed3beca11c11a

folly/fibers/TimedMutex-inl.h

@@ -149,13 +149,19 @@ inline void TimedMutex::unlock() {
 }
 
 //
-// TimedRWMutex implementation
+// TimedRWMutexImpl implementation
 //
 
-template <typename BatonType>
-void TimedRWMutex<BatonType>::read_lock() {
+template <bool ReaderPriority, typename BatonType>
+bool TimedRWMutexImpl<ReaderPriority, BatonType>::shouldReadersWait() const {
+  return state_ == State::WRITE_LOCKED ||
+      (!ReaderPriority && !write_waiters_.empty());
+}
+
+template <bool ReaderPriority, typename BatonType>
+void TimedRWMutexImpl<ReaderPriority, BatonType>::read_lock() {
   std::unique_lock<folly::SpinLock> ulock{lock_};
-  if (state_ == State::WRITE_LOCKED) {
+  if (shouldReadersWait()) {
     MutexWaiter waiter;
     read_waiters_.push_back(waiter);
     ulock.unlock();
@@ -171,12 +177,12 @@ void TimedRWMutex<BatonType>::read_lock() {
   readers_ += 1;
 }
 
-template <typename BatonType>
+template <bool ReaderPriority, typename BatonType>
 template <typename Rep, typename Period>
-bool TimedRWMutex<BatonType>::timed_read_lock(
+bool TimedRWMutexImpl<ReaderPriority, BatonType>::timed_read_lock(
     const std::chrono::duration<Rep, Period>& duration) {
   std::unique_lock<folly::SpinLock> ulock{lock_};
-  if (state_ == State::WRITE_LOCKED) {
+  if (shouldReadersWait()) {
     MutexWaiter waiter;
     read_waiters_.push_back(waiter);
     ulock.unlock();
@@ -204,10 +210,10 @@ bool TimedRWMutex<BatonType>::timed_read_lock(
   return true;
 }
 
-template <typename BatonType>
-bool TimedRWMutex<BatonType>::try_read_lock() {
+template <bool ReaderPriority, typename BatonType>
+bool TimedRWMutexImpl<ReaderPriority, BatonType>::try_read_lock() {
   std::lock_guard<SpinLock> guard{lock_};
-  if (state_ != State::WRITE_LOCKED) {
+  if (!shouldReadersWait()) {
     assert(
         (state_ == State::UNLOCKED && readers_ == 0) ||
         (state_ == State::READ_LOCKED && readers_ > 0));
@@ -219,8 +225,8 @@ bool TimedRWMutex<BatonType>::try_read_lock() {
   return false;
 }
 
-template <typename BatonType>
-void TimedRWMutex<BatonType>::write_lock() {
+template <bool ReaderPriority, typename BatonType>
+void TimedRWMutexImpl<ReaderPriority, BatonType>::write_lock() {
   std::unique_lock<folly::SpinLock> ulock{lock_};
   if (state_ == State::UNLOCKED) {
     verify_unlocked_properties();
@@ -233,9 +239,9 @@ void TimedRWMutex<BatonType>::write_lock() {
   waiter.baton.wait();
 }
 
-template <typename BatonType>
+template <bool ReaderPriority, typename BatonType>
 template <typename Rep, typename Period>
-bool TimedRWMutex<BatonType>::timed_write_lock(
+bool TimedRWMutexImpl<ReaderPriority, BatonType>::timed_write_lock(
     const std::chrono::duration<Rep, Period>& duration) {
   std::unique_lock<folly::SpinLock> ulock{lock_};
   if (state_ == State::UNLOCKED) {
@@ -263,8 +269,8 @@ bool TimedRWMutex<BatonType>::timed_write_lock(
   return true;
 }
 
-template <typename BatonType>
-bool TimedRWMutex<BatonType>::try_write_lock() {
+template <bool ReaderPriority, typename BatonType>
+bool TimedRWMutexImpl<ReaderPriority, BatonType>::try_write_lock() {
   std::lock_guard<SpinLock> guard{lock_};
   if (state_ == State::UNLOCKED) {
     verify_unlocked_properties();
@@ -274,8 +280,8 @@ bool TimedRWMutex<BatonType>::try_write_lock() {
   return false;
 }
 
-template <typename BatonType>
-void TimedRWMutex<BatonType>::unlock() {
+template <bool ReaderPriority, typename BatonType>
+void TimedRWMutexImpl<ReaderPriority, BatonType>::unlock() {
   std::lock_guard<SpinLock> guard{lock_};
   assert(state_ != State::UNLOCKED);
   assert(
@@ -285,7 +291,7 @@ void TimedRWMutex<BatonType>::unlock() {
     readers_ -= 1;
   }
 
-  if (!read_waiters_.empty()) {
+  if (!read_waiters_.empty() && (ReaderPriority || write_waiters_.empty())) {
     assert(
         state_ == State::WRITE_LOCKED && readers_ == 0 &&
         "read waiters can only accumulate while write locked");
@@ -299,7 +305,7 @@ void TimedRWMutex<BatonType>::unlock() {
     }
   } else if (readers_ == 0) {
     if (!write_waiters_.empty()) {
-      assert(read_waiters_.empty());
+      assert(read_waiters_.empty() || !ReaderPriority);
       state_ = State::WRITE_LOCKED;
 
       // Wake a single writer (after releasing the spin lock)
@@ -315,8 +321,8 @@ void TimedRWMutex<BatonType>::unlock() {
   }
 }
 
-template <typename BatonType>
-void TimedRWMutex<BatonType>::downgrade() {
+template <bool ReaderPriority, typename BatonType>
+void TimedRWMutexImpl<ReaderPriority, BatonType>::downgrade() {
   std::lock_guard<SpinLock> guard{lock_};
   assert(state_ == State::WRITE_LOCKED && readers_ == 0);
   state_ = State::READ_LOCKED;

folly/fibers/TimedMutex.h

@@ -80,24 +80,32 @@ class TimedMutex {
 };
 
 /**
- * @class TimedRWMutex
+ * @class TimedRWMutexImpl
  *
  * A readers-writer lock which allows multiple readers to hold the
  * lock simultaneously or only one writer.
  *
- * NOTE: This is a reader-preferred RWLock i.e. readers are give priority
- * when there are both readers and writers waiting to get the lock.
+ * NOTE: When ReaderPriority is set to true then the lock is a reader-preferred
+ * RWLock i.e. readers are given priority when there are both readers and
+ * writers waiting to get the lock.
+ *
+ * When ReaderPriority is set to false then the lock is a writer-preferred
+ * RWLock i.e. writers are given priority when there are both readers and
+ * writers waiting to get the lock. Note that when the lock is in
+ * writer-preferred mode, the readers are not re-entrant (e.g. if a caller owns
+ * a read lock, it can't attempt to acquire the read lock again as it can
+ * deadlock.)
  **/
-template <typename BatonType>
-class TimedRWMutex {
+template <bool ReaderPriority, typename BatonType>
+class TimedRWMutexImpl {
  public:
-  TimedRWMutex() = default;
-  ~TimedRWMutex() = default;
+  TimedRWMutexImpl() = default;
+  ~TimedRWMutexImpl() = default;
 
-  TimedRWMutex(const TimedRWMutex& rhs) = delete;
-  TimedRWMutex& operator=(const TimedRWMutex& rhs) = delete;
-  TimedRWMutex(TimedRWMutex&& rhs) = delete;
-  TimedRWMutex& operator=(TimedRWMutex&& rhs) = delete;
+  TimedRWMutexImpl(const TimedRWMutexImpl& rhs) = delete;
+  TimedRWMutexImpl& operator=(const TimedRWMutexImpl& rhs) = delete;
+  TimedRWMutexImpl(TimedRWMutexImpl&& rhs) = delete;
+  TimedRWMutexImpl& operator=(TimedRWMutexImpl&& rhs) = delete;
 
   // Lock for shared access. The thread / fiber is blocked until the lock
   // can be acquired.
@@ -143,7 +151,7 @@ class TimedRWMutex {
 
   class ReadHolder {
    public:
-    explicit ReadHolder(TimedRWMutex& lock) : lock_(&lock) {
+    explicit ReadHolder(TimedRWMutexImpl& lock) : lock_(&lock) {
       lock_->read_lock();
     }
 
@@ -159,12 +167,12 @@ class TimedRWMutex {
     ReadHolder& operator=(ReadHolder&& rhs) = delete;
 
    private:
-    TimedRWMutex* lock_;
+    TimedRWMutexImpl* lock_;
   };
 
   class WriteHolder {
    public:
-    explicit WriteHolder(TimedRWMutex& lock) : lock_(&lock) {
+    explicit WriteHolder(TimedRWMutexImpl& lock) : lock_(&lock) {
       lock_->write_lock();
     }
 
@@ -180,7 +188,7 @@ class TimedRWMutex {
     WriteHolder& operator=(WriteHolder&& rhs) = delete;
 
    private:
-    TimedRWMutex* lock_;
+    TimedRWMutexImpl* lock_;
   };
 
  private:
@@ -191,6 +199,8 @@ class TimedRWMutex {
     assert(write_waiters_.empty());
   }
 
+  bool shouldReadersWait() const;
+
   // Different states the lock can be in
   enum class State {
     UNLOCKED,
@@ -228,6 +238,16 @@ class TimedRWMutex {
   MutexWaiterList read_waiters_; //< List of thread / fibers waiting for
   //  shared access
 };
+
+template <typename BatonType>
+using TimedRWMutexReadPriority = TimedRWMutexImpl<true, BatonType>;
+
+template <typename BatonType>
+using TimedRWMutexWritePriority = TimedRWMutexImpl<false, BatonType>;
+
+template <typename BatonType>
+using TimedRWMutex = TimedRWMutexReadPriority<BatonType>;
+
 } // namespace fibers
 } // namespace folly