From 362e3dbaa4da521a8d3c6ffc977c81a9d0219e5a Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Wed, 15 Aug 2018 19:02:23 +0800 Subject: [PATCH] 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 to offer the alias of std::mutex instead inheriting from it as SharedMutex, is that, std::condition_variable::wait() requires `std::unique_lock&`, not `std::unique_lock&` as its parameter. Signed-off-by: Kefu Chai --- src/common/lock_cond.h | 41 ++++----- src/common/lock_mutex.h | 62 +++++++------ src/common/lock_shared_mutex.h | 67 ++++++++++++++ src/common/shared_cache.hpp | 133 +++++++++++++-------------- src/test/common/test_shared_cache.cc | 18 ++-- 5 files changed, 194 insertions(+), 127 deletions(-) create mode 100644 src/common/lock_shared_mutex.h diff --git a/src/common/lock_cond.h b/src/common/lock_cond.h index acfb778fa8a80..2cf067c80c556 100644 --- 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 +#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 class LockCond { -public: - int Wait(LockMutex&) { - return 0; - } - int Signal() { - return 0; - } + // lockless condition_variable cannot be represented using + // std::condition_variables interfaces. }; +#ifdef NDEBUG +template<> +class LockCond +{ +public: + using type = std::condition_variable; +}; +#else template<> class LockCond { public: - int Wait(LockMutex& 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 using LockCondT = typename LockCond::type; +} // namespace ceph diff --git a/src/common/lock_mutex.h b/src/common/lock_mutex.h index 793ec964efed6..8203d8d3edfcf 100644 --- 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 +#else +#include "mutex_debug.h" +#endif namespace ceph { -template class LockCond; - // empty helper class except when the template argument is LockPolicy::MUTEX template 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 - 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 { public: + using type = std::mutex; + // discard the constructor params template - LockMutex(Args&& ...args) - : mutex{std::forward(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 { +public: + using type = ceph::mutex_debug; + template + static ceph::mutex_debug create(Args&& ...args) { + return ceph::mutex_debug{std::forward(args)...}; } - mutable Mutex mutex; - friend class LockCond; - friend class ::SharedLRUTest; }; +#endif // NDEBUG + +template using LockMutexT = typename LockMutex::type; } // namespace ceph diff --git a/src/common/lock_shared_mutex.h b/src/common/lock_shared_mutex.h new file mode 100644 index 0000000000000..a5b0be1299f9a --- /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 +#else +#include "shared_mutex_debug.h" +#endif + +namespace ceph { + +// empty helper class except when the template argument is LockPolicy::MUTEX +template +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 + static Dummy create(Args&& ...) { + return Dummy{}; + } +}; + +#ifdef NDEBUG +template<> +class SharedMutex +{ +public: + using type = std::shared_mutex; + // discard the constructor params + template + static std::shared_mutex create(Args&&... args) { + return std::shared_mutex{}; + } +}; +#else +template<> +class SharedMutex { +public: + using type = ceph::shared_mutex_debug; + template + static ceph::shared_mutex_debug create(Args&&... args) { + return ceph::shared_mutex_debug{std::forward(args)...}; + } +}; +#endif // NDEBUG + +template using SharedMutexT = typename SharedMutex::type; + +} // namespace ceph + diff --git a/src/common/shared_cache.hpp b/src/common/shared_cache.hpp index 5eeb4724933fd..2fdfff5813607 100644 --- a/src/common/shared_cache.hpp +++ b/src/common/shared_cache.hpp @@ -33,7 +33,7 @@ class SharedLRU { using shared_ptr_trait_t = SharedPtrTrait; using VPtr = typename shared_ptr_trait_t::template shared_ptr; using WeakVPtr = typename shared_ptr_trait_t::template weak_ptr; - LockMutex lock; + LockMutexT lock; size_t max_size; LockCond 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, 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::create("SharedLRU::lock")}, + max_size(max_size), size(0), waiting(0) { contents.rehash(max_size); } @@ -123,7 +125,7 @@ public: // reorder. map, 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, 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, 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 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 to_release; { - auto locker = lock(); + std::unique_lock l{lock}; ++waiting; - bool retry = false; - do { - retry = false; - if (weak_refs.empty()) - break; - typename map, 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 *next) { pair r; { - auto locker = lock(); + std::lock_guard l{lock}; VPtr next_val; typename map, C>::iterator i = weak_refs.upper_bound(key); @@ -273,23 +272,20 @@ public: VPtr val; list to_release; { - auto locker = lock(); + std::unique_lock l{lock}; ++waiting; - bool retry = false; - do { - retry = false; - typename map, 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 to_release; { - auto locker = lock(); - bool retry = false; - do { - retry = false; - typename map, 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 to_release; { - auto locker = lock(); + std::lock_guard l{lock}; typename map, 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 ea0ba288e42b0..8db5d9ce37c7e 100644 --- a/src/test/common/test_shared_cache.cc +++ b/src/test/common/test_shared_cache.cc @@ -28,8 +28,8 @@ class SharedLRUTest : public SharedLRU { public: - Mutex &get_lock() { return lock.get(); } - Cond &get_cond() { return cond.get(); } + auto& get_lock() { return lock; } + auto& get_cond() { return cond; } map, 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(); -- 2.39.5