From d5b3b2c7b68a0b493384caabcb6f4a2549449b92 Mon Sep 17 00:00:00 2001
From: Alexander Kazakov <Akazz@users.noreply.github.com>
Date: Thu, 16 Apr 2020 21:56:21 +0300
Subject: [PATCH] Updated RWLockImpl: Fixed suboptimacy (readers queuing up) +
 minor tweaks to names and comments (#10303)

* Readers queue correction
* Comments, renamings and cosmetics
---
 src/Common/RWLock.cpp | 29 ++++++++---------------------
 src/Common/RWLock.h   | 12 +++++++++---
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/src/Common/RWLock.cpp b/src/Common/RWLock.cpp
index d6b8cbd244f..a8dba490fac 100644
--- a/src/Common/RWLock.cpp
+++ b/src/Common/RWLock.cpp
@@ -154,23 +154,17 @@ RWLockImpl::getLock(RWLockImpl::Type type, const String & query_id, const std::c
         writers_queue.emplace_back(type);  /// SM1: may throw (nothing to roll back)
     }
     else if (readers_queue.empty() ||
-            (rdlock_owner == readers_queue.begin() && !writers_queue.empty()))
+            (rdlock_owner == readers_queue.begin() && readers_queue.size() == 1 && !writers_queue.empty()))
     {
         readers_queue.emplace_back(type);  /// SM1: may throw (nothing to roll back)
     }
     GroupsContainer::iterator it_group =
             (type == Type::Write) ? std::prev(writers_queue.end()) : std::prev(readers_queue.end());
 
+    /// Lock is free to acquire
     if (rdlock_owner == readers_queue.end() && wrlock_owner == writers_queue.end())
     {
-        if (type == Type::Read)
-        {
-            rdlock_owner = it_group;  /// SM2: nothrow
-        }
-        else
-        {
-            wrlock_owner = it_group;  /// SM2: nothrow
-        }
+        (type == Read ? rdlock_owner : wrlock_owner) = it_group;  /// SM2: nothrow
     }
     else
     {
@@ -192,17 +186,10 @@ RWLockImpl::getLock(RWLockImpl::Type type, const String & query_id, const std::c
             /// Step 3a. Check if we must handle timeout and exit
             if (!wait_result)  /// Wait timed out!
             {
+                /// Rollback(SM1): nothrow
                 if (it_group->requests == 0)
                 {
-                    /// Roll back SM1
-                    if (type == Read)
-                    {
-                        readers_queue.erase(it_group);  /// Rollback(SM1): nothrow
-                    }
-                    else
-                    {
-                        writers_queue.erase(it_group);  /// Rollback(SM1): nothrow
-                    }
+                    (type == Read ? readers_queue : writers_queue).erase(it_group);
                 }
 
                 return nullptr;
@@ -224,7 +211,7 @@ RWLockImpl::getLock(RWLockImpl::Type type, const String & query_id, const std::c
             /// Methods std::list<>::emplace_back() and std::unordered_map<>::emplace() provide strong exception safety
             /// We only need to roll back the changes to these objects: owner_queries and the readers/writers queue
             if (it_group->requests == 0)
-                eraseGroup(it_group);  /// Rollback(SM1): nothrow
+                dropOwnerGroupAndPassOwnership(it_group);  /// Rollback(SM1): nothrow
 
             throw;
         }
@@ -272,11 +259,11 @@ void RWLockImpl::unlock(GroupsContainer::iterator group_it, const String & query
 
     /// If we are the last remaining referrer, remove this QNode and notify the next one
     if (--group_it->requests == 0)               /// SM: nothrow
-        eraseGroup(group_it);
+        dropOwnerGroupAndPassOwnership(group_it);
 }
 
 
-void RWLockImpl::eraseGroup(GroupsContainer::iterator group_it) noexcept
+void RWLockImpl::dropOwnerGroupAndPassOwnership(GroupsContainer::iterator group_it) noexcept
 {
     rdlock_owner = readers_queue.end();
     wrlock_owner = writers_queue.end();
diff --git a/src/Common/RWLock.h b/src/Common/RWLock.h
index 43366192cf8..ad0a3f139fc 100644
--- a/src/Common/RWLock.h
+++ b/src/Common/RWLock.h
@@ -19,9 +19,15 @@ class RWLockImpl;
 using RWLock = std::shared_ptr<RWLockImpl>;
 
 
-/// Implements shared lock with FIFO service
+/// Implements Readers-Writers locking algorithm that serves requests in "Phase Fair" order.
 /// (Phase Fair RWLock as suggested in https://www.cs.unc.edu/~anderson/papers/rtsj10-for-web.pdf)
-/// Can be acquired recursively (for the same query) in Read mode
+/// It is used for synchronizing access to various objects on query level (i.e. Storages).
+///
+/// In general, ClickHouse processes queries by multiple threads of execution in parallel.
+/// As opposed to the standard OS synchronization primitives (mutexes), this implementation allows
+/// unlock() to be called by a thread other than the one, that called lock().
+/// It is also possible to acquire RWLock in Read mode without waiting (FastPath) by multiple threads,
+/// that execute the same query (share the same query_id).
 ///
 /// NOTE: it is important to allow acquiring the same lock in Read mode without waiting if it is already
 /// acquired by another thread of the same query. Otherwise the following deadlock is possible:
@@ -82,6 +88,6 @@ private:
 private:
     RWLockImpl() = default;
     void unlock(GroupsContainer::iterator group_it, const String & query_id) noexcept;
-    void eraseGroup(GroupsContainer::iterator group_it) noexcept;
+    void dropOwnerGroupAndPassOwnership(GroupsContainer::iterator group_it) noexcept;
 };
 }