common: use std::mutex and friends if NDEBUG

revise LockCond and its friends to satisfy the concepts of their
counterparts in C++ standard library, so they can used as drop-in
replacements of them.

and use std::mutex, std::condition_variables, std::shared_mutex
if NDEBUG is defined for better performance.

the reason i specialize LockMutex<LockPolicy::MUTEX> to offer the
alias of std::mutex instead inheriting from it as SharedMutex,
is that, std::condition_variable::wait() requires
`std::unique_lock<std::mutex>&`, not
`std::unique_lock<subclass_of_mutex>&` as its parameter.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/common/lock_cond.h b/src/common/lock_cond.h
index acfb778fa8a80e91e0a6181b25a3678c27ca0db6..2cf067c80c5566afeff12e724d56d6ed9c5020cf 100644 (file)
--- a/src/common/lock_cond.h
+++ b/src/common/lock_cond.h
@@ -4,7 +4,11 @@
 
 #include "lock_policy.h"
 #include "lock_mutex.h"
-#include "Cond.h"
+#ifdef NDEBUG
+#include <condition_variable>
+#else
+#include "common/condition_variable_debug.h"
+#endif
 
 class SharedLRUTest;
 
@@ -13,31 +17,26 @@ namespace ceph {
 // empty helper class except when the template argument is LockPolicy::MUTEX
 template<LockPolicy lock_policy>
 class LockCond {
-public:
-  int Wait(LockMutex<lock_policy>&) {
-    return 0;
-  }
-  int Signal() {
-    return 0;
-  }
+  // lockless condition_variable cannot be represented using
+  // std::condition_variables interfaces.
 };
 
+#ifdef NDEBUG
+template<>
+class LockCond<LockPolicy::MUTEX>
+{
+public:
+  using type = std::condition_variable;
+};
+#else
 template<>
 class LockCond<LockPolicy::MUTEX> {
 public:
-  int Wait(LockMutex<LockPolicy::MUTEX>& mutex) {
-    return cond.Wait(mutex.get());
-  }
-  int Signal() {
-    return cond.Signal();
-  }
-private:
-  Cond& get() {
-    return cond;
-  }
-  Cond cond;
-  friend class ::SharedLRUTest;
+  using type = ceph::condition_variable_debug;
 };
 
-} // namespace ceph
+#endif // NDEBUG
+
+template<LockPolicy lp> using LockCondT = typename LockCond<lp>::type;
 
+} // namespace ceph

diff --git a/src/common/lock_mutex.h b/src/common/lock_mutex.h
index 793ec964efed67ad7d2a03061e2997965e62d68f..8203d8d3edfcf938636c3a91d1cabde165758c84 100644 (file)
--- a/src/common/lock_mutex.h
+++ b/src/common/lock_mutex.h
@@ -3,49 +3,59 @@
 #pragma once
 
 #include "lock_policy.h"
-#include "Mutex.h"
-
-class SharedLRUTest;
+#ifdef NDEBUG
+#include <mutex>
+#else
+#include "mutex_debug.h"
+#endif
 
 namespace ceph {
 
-template<LockPolicy lp> class LockCond;
-
 // empty helper class except when the template argument is LockPolicy::MUTEX
 template<LockPolicy lp>
 class LockMutex {
+  struct Dummy {
+    void lock() {}
+    bool try_lock() {
+      return true;
+    }
+    void unlock() {}
+    bool is_locked() const {
+      return true;
+    }
+  };
 public:
+  using type = Dummy;
+  // discard the constructor params
   template<typename... Args>
-  LockMutex(Args&& ...) {}
-  auto operator()() const {
-    struct Locker {};
-    return Locker{};
-  }
-  bool is_locked() const {
-    return true;
+  static Dummy create(Args&& ...) {
+    return Dummy{};
   }
 };
 
+#ifdef NDEBUG
 template<>
 class LockMutex<LockPolicy::MUTEX> {
 public:
+  using type = std::mutex;
+  // discard the constructor params
   template<typename... Args>
-  LockMutex(Args&& ...args)
-    : mutex{std::forward<Args>(args)...}
-  {}
-  auto operator()() const {
-    return Mutex::Locker{mutex};
+  static std::mutex create(Args&& ...) {
+    return std::mutex{};
   }
-  bool is_locked() const {
-    return mutex.is_locked();
-  }
-private:
-  Mutex& get() {
-    return mutex;
+};
+#else
+template<>
+class LockMutex<LockPolicy::MUTEX> {
+public:
+  using type = ceph::mutex_debug;
+  template<typename... Args>
+  static ceph::mutex_debug create(Args&& ...args) {
+    return ceph::mutex_debug{std::forward<Args>(args)...};
   }
-  mutable Mutex mutex;
-  friend class LockCond<LockPolicy::MUTEX>;
-  friend class ::SharedLRUTest;
 };
+#endif // NDEBUG
+
+template<LockPolicy lp> using LockMutexT = typename LockMutex<lp>::type;
 
 } // namespace ceph

diff --git a/src/common/lock_shared_mutex.h b/src/common/lock_shared_mutex.h
new file mode 100644 (file)
index 0000000..a5b0be1
--- /dev/null
+++ b/src/common/lock_shared_mutex.h
@@ -0,0 +1,67 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#pragma once
+
+#include "lock_policy.h"
+#ifdef NDEBUG
+#include <shared_mutex>
+#else
+#include "shared_mutex_debug.h"
+#endif
+
+namespace ceph {
+
+// empty helper class except when the template argument is LockPolicy::MUTEX
+template<LockPolicy lp>
+class SharedMutex {
+  struct Dummy {
+    // exclusive lock
+    void lock() {}
+    bool try_lock() {
+      return true;
+    }
+    void unlock() {}
+    // shared lock
+    void lock_shared() {}
+    bool try_lock_shared() {
+      return true;
+    }
+    void unlock_shared() {}
+  };
+public:
+  using type = Dummy;
+  template<typename... Args>
+  static Dummy create(Args&& ...) {
+    return Dummy{};
+  }
+};
+
+#ifdef NDEBUG
+template<>
+class SharedMutex<LockPolicy::MUTEX>
+{
+public:
+  using type = std::shared_mutex;
+  // discard the constructor params
+  template<typename... Args>
+  static std::shared_mutex create(Args&&... args) {
+    return std::shared_mutex{};
+  }
+};
+#else
+template<>
+class SharedMutex<LockPolicy::MUTEX> {
+public:
+  using type = ceph::shared_mutex_debug;
+  template<typename... Args>
+  static ceph::shared_mutex_debug create(Args&&... args) {
+    return ceph::shared_mutex_debug{std::forward<Args>(args)...};
+  }
+};
+#endif // NDEBUG
+
+template<LockPolicy lp> using SharedMutexT = typename SharedMutex<lp>::type;
+
+} // namespace ceph
+

diff --git a/src/common/shared_cache.hpp b/src/common/shared_cache.hpp
index 5eeb4724933fd290f0d617b18c7b076e4ca2cd38..2fdfff581360751146017ae618fa50246ca30be4 100644 (file)
--- a/src/common/shared_cache.hpp
+++ b/src/common/shared_cache.hpp
@@ -33,7 +33,7 @@ class SharedLRU {
   using shared_ptr_trait_t = SharedPtrTrait<lock_policy>;
   using VPtr = typename shared_ptr_trait_t::template shared_ptr<V>;
   using WeakVPtr = typename shared_ptr_trait_t::template weak_ptr<V>;
-  LockMutex<lock_policy> lock;
+  LockMutexT<lock_policy> lock;
   size_t max_size;
   LockCond<lock_policy> cond;
   unsigned size;
@@ -78,12 +78,12 @@ private:
   }
 
   void remove(const K& key, V *valptr) {
-    auto locker = lock();
+    std::lock_guard l{lock};
     typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.find(key);
     if (i != weak_refs.end() && i->second.second == valptr) {
       weak_refs.erase(i);
     }
-    cond.Signal();
+    cond.notify_one();
   }
 
   class Cleanup {
@@ -99,7 +99,9 @@ private:
 
 public:
   SharedLRU(CephContext *cct = NULL, size_t max_size = 20)
-    : cct(cct), lock("SharedLRU::lock"), max_size(max_size), 
+    : cct(cct),
+      lock{LockMutex<lock_policy>::create("SharedLRU::lock")},
+      max_size(max_size),
       size(0), waiting(0) {
     contents.rehash(max_size); 
   }
@@ -123,7 +125,7 @@ public:
     // reorder.
     map<K, pair<WeakVPtr, V*>, C> temp;
 
-    auto locker = lock();
+    std::lock_guard locker{lock};
     temp.swap(weak_refs);
 
     // reconstruct with new comparator
@@ -160,7 +162,7 @@ public:
   void clear() {
     while (true) {
       VPtr val; // release any ref we have after we drop the lock
-      auto locker = lock();
+      std::lock_guard locker{lock};
       if (size == 0)
         break;
 
@@ -172,7 +174,7 @@ public:
   void clear(const K& key) {
     VPtr val; // release any ref we have after we drop the lock
     {
-      auto locker = lock();
+      std::lock_guard l{lock};
       typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.find(key);
       if (i != weak_refs.end()) {
        val = i->second.first.lock();
@@ -184,7 +186,7 @@ public:
   void purge(const K &key) {
     VPtr val; // release any ref we have after we drop the lock
     {
-      auto locker = lock();
+      std::lock_guard l{lock};
       typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.find(key);
       if (i != weak_refs.end()) {
        val = i->second.first.lock();
@@ -197,7 +199,7 @@ public:
   void set_size(size_t new_size) {
     list<VPtr> to_release;
     {
-      auto locker = lock();
+      std::lock_guard l{lock};
       max_size = new_size;
       trim_cache(&to_release);
     }
@@ -205,7 +207,7 @@ public:
 
   // Returns K key s.t. key <= k for all currently cached k,v
   K cached_key_lower_bound() {
-    auto locker = lock();
+    std::lock_guard l{lock};
     return weak_refs.begin()->first;
   }
 
@@ -213,26 +215,23 @@ public:
     VPtr val;
     list<VPtr> to_release;
     {
-      auto locker = lock();
+      std::unique_lock l{lock};
       ++waiting;
-      bool retry = false;
-      do {
-       retry = false;
-       if (weak_refs.empty())
-         break;
-       typename map<K, pair<WeakVPtr, V*>, C>::iterator i =
-         weak_refs.lower_bound(key);
-       if (i == weak_refs.end())
-         --i;
-       val = i->second.first.lock();
-       if (val) {
-         lru_add(i->first, val, &to_release);
-       } else {
-         retry = true;
-       }
-       if (retry)
-         cond.Wait(lock);
-      } while (retry);
+      cond.wait(l, [this, &key, &val, &to_release] {
+        if (weak_refs.empty()) {
+          return true;
+        }
+        auto i = weak_refs.lower_bound(key);
+        if (i == weak_refs.end()) {
+          --i;
+        }
+        if (val = i->second.first.lock(); val) {
+          lru_add(i->first, val, &to_release);
+          return true;
+        } else {
+          return false;
+        }
+      });
       --waiting;
     }
     return val;
@@ -240,7 +239,7 @@ public:
   bool get_next(const K &key, pair<K, VPtr> *next) {
     pair<K, VPtr> r;
     {
-      auto locker = lock();
+      std::lock_guard l{lock};
       VPtr next_val;
       typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.upper_bound(key);
 
@@ -273,23 +272,20 @@ public:
     VPtr val;
     list<VPtr> to_release;
     {
-      auto locker = lock();
+      std::unique_lock l{lock};
       ++waiting;
-      bool retry = false;
-      do {
-       retry = false;
-       typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.find(key);
-       if (i != weak_refs.end()) {
-         val = i->second.first.lock();
-         if (val) {
-           lru_add(key, val, &to_release);
-         } else {
-           retry = true;
-         }
-       }
-       if (retry)
-         cond.Wait(lock);
-      } while (retry);
+      cond.wait(l, [this, &key, &val, &to_release] {
+        if (auto i = weak_refs.find(key); i != weak_refs.end()) {
+          if (val = i->second.first.lock(); val) {
+            lru_add(key, val, &to_release);
+            return true;
+          } else {
+            return false;
+          }
+        } else {
+          return true;
+        }
+      });
       --waiting;
     }
     return val;
@@ -298,30 +294,25 @@ public:
     VPtr val;
     list<VPtr> to_release;
     {
-      auto locker = lock();
-      bool retry = false;
-      do {
-       retry = false;
-       typename map<K, pair<WeakVPtr, V*>, C>::iterator i = weak_refs.find(key);
-       if (i != weak_refs.end()) {
-         val = i->second.first.lock();
-         if (val) {
-           lru_add(key, val, &to_release);
-           return val;
-         } else {
-           retry = true;
-         }
-       }
-       if (retry)
-         cond.Wait(lock);
-      } while (retry);
-
-      V *new_value = new V();
-      VPtr new_val(new_value, Cleanup(this, key));
-      weak_refs.insert(make_pair(key, make_pair(new_val, new_value)));
-      lru_add(key, new_val, &to_release);
-      return new_val;
+      std::unique_lock l{lock};
+      cond.wait(l, [this, &key, &val] {
+        if (auto i = weak_refs.find(key); i != weak_refs.end()) {
+          if (val = i->second.first.lock(); val) {
+            return true;
+          } else {
+            return false;
+          }
+        } else {
+          return true;
+        }
+      });
+      if (!val) {
+        val = VPtr{new V{}, Cleanup{this, key}};
+        weak_refs.insert(make_pair(key, make_pair(val, val.get())));
+      }
+      lru_add(key, val, &to_release);
     }
+    return val;
   }
 
   /**
@@ -331,7 +322,7 @@ public:
    * in the cache.
    */
   bool empty() {
-    auto locker = lock();
+    std::lock_guard l{lock};
     return weak_refs.empty();
   }
 
@@ -351,7 +342,7 @@ public:
     VPtr val;
     list<VPtr> to_release;
     {
-      auto locker = lock();
+      std::lock_guard l{lock};
       typename map<K, pair<WeakVPtr, V*>, C>::iterator actual =
        weak_refs.lower_bound(key);
       if (actual != weak_refs.end() && actual->first == key) {

diff --git a/src/test/common/test_shared_cache.cc b/src/test/common/test_shared_cache.cc
index ea0ba288e42b06d7f5dfc163b39a489e53908cc4..8db5d9ce37c7ecdd095ef2b66edc5276acaf8531 100644 (file)
--- a/src/test/common/test_shared_cache.cc
+++ b/src/test/common/test_shared_cache.cc
@@ -28,8 +28,8 @@
 
 class SharedLRUTest : public SharedLRU<unsigned int, int> {
 public:
-  Mutex &get_lock() { return lock.get(); }
-  Cond &get_cond() { return cond.get(); }
+  auto& get_lock() { return lock; }
+  auto& get_cond() { return cond; }
   map<unsigned int, pair< std::weak_ptr<int>, int* > > &get_weak_refs() {
     return weak_refs;
   }
@@ -82,7 +82,7 @@ public:
       if (delay > 0)
         usleep(delay);
       {
-        Mutex::Locker l(cache.get_lock());
+        std::lock_guard l{cache.get_lock()};
         if (cache.waiting == waitting) {
           break;
         }
@@ -179,9 +179,9 @@ TEST_F(SharedLRU_all, wait_lookup) {
   // waiting on a key does not block lookups on other keys
   EXPECT_FALSE(cache.lookup(key + 12345));
   {
-    Mutex::Locker l(cache.get_lock());
+    std::lock_guard l{cache.get_lock()};
     cache.get_weak_refs().erase(key);
-    cache.get_cond().Signal();
+    cache.get_cond().notify_one();
   }
   ASSERT_TRUE(wait_for(cache, 0));
   t.join();
@@ -205,9 +205,9 @@ TEST_F(SharedLRU_all, wait_lookup_or_create) {
   // waiting on a key does not block lookups on other keys
   EXPECT_TRUE(cache.lookup_or_create(key + 12345).get());
   {
-    Mutex::Locker l(cache.get_lock());
+    std::lock_guard l{cache.get_lock()};
     cache.get_weak_refs().erase(key);
-    cache.get_cond().Signal();
+    cache.get_cond().notify_one();
   }
   ASSERT_TRUE(wait_for(cache, 0));
   t.join();
@@ -250,9 +250,9 @@ TEST_F(SharedLRU_all, wait_lower_bound) {
   // waiting on a key does not block getting lower_bound on other keys
   EXPECT_TRUE(cache.lower_bound(other_key).get());
   {
-    Mutex::Locker l(cache.get_lock());
+    std::lock_guard l{cache.get_lock()};
     cache.get_weak_refs().erase(key);
-    cache.get_cond().Signal();
+    cache.get_cond().notify_one();
   }
   ASSERT_TRUE(wait_for(cache, 0));
   t.join();