From b179601dfc42d8e3230d6477d7db8f3d5a8f64dc Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Mon, 11 Jul 2016 18:36:01 -0700 Subject: [PATCH] add Synchronized::withLock() methods Summary: Add withLock() and related methods for invoking a lambda function while the lock is held. This is sometimes more convenient than opening a new scope and using lock(). withLock() also retains some of the benefits of the SYNCHRONIZED macro in that it forces users to put their critical section code in a new scope, making the critical section more visibly distinct in the code. This also encourages users to only put necessary work inside the critical section, and do to other work once the lock is released. This also adds a LockedGuardPtr class, which is a slightly cheaper version of LockedPtr. The relationship between LockedGuardPtr and LockedPtr is very much like that between std::lock_guard and std::unique_lock. It saves a branch in the destructor, and in the case of std::mutex it also saves a small amount of storage space (since LockedPtr is specialized for std::mutex to also store a std::unique_lock). Reviewed By: yfeldblum, djwatson Differential Revision: D3530368 fbshipit-source-id: 72a4f457b3f18e8e8f4cc6713218f6882bb89818 --- folly/Synchronized.h | 214 +++++++++++++++++++++++++-- folly/test/SynchronizedTest.cpp | 4 + folly/test/SynchronizedTestLib-inl.h | 163 +++++++++++++++++++- 3 files changed, 366 insertions(+), 15 deletions(-) diff --git a/folly/Synchronized.h b/folly/Synchronized.h index f36bf559..01619bc2 100644 --- a/folly/Synchronized.h +++ b/folly/Synchronized.h @@ -39,6 +39,8 @@ template class LockedPtrBase; template class LockedPtr; +template +class LockedGuardPtr; /** * SynchronizedBase is a helper parent class for Synchronized. @@ -112,6 +114,75 @@ class SynchronizedBase { const std::chrono::duration& timeout) const { return ConstLockedPtr(static_cast(this), timeout); } + + /* + * Note: C++ 17 adds guaranteed copy elision. (http://wg21.link/P0135) + * Once compilers support this, it would be nice to add wguard() and rguard() + * methods that return LockedGuardPtr objects. + */ + + /** + * Invoke a function while holding the lock exclusively. + * + * A reference to the datum will be passed into the function as its only + * argument. + * + * This can be used with a lambda argument for easily defining small critical + * sections in the code. For example: + * + * auto value = obj.withWLock([](auto& data) { + * data.doStuff(); + * return data.getValue(); + * }); + */ + template + auto withWLock(Function&& function) { + LockedGuardPtr guardPtr( + static_cast(this)); + return function(*guardPtr); + } + template + auto withWLock(Function&& function) const { + LockedGuardPtr guardPtr( + static_cast(this)); + return function(*guardPtr); + } + + /** + * Invoke a function while holding the lock exclusively. + * + * This is similar to withWLock(), but the function will be passed a + * LockedPtr rather than a reference to the data itself. + * + * This allows scopedUnlock() to be called on the LockedPtr argument if + * desired. + */ + template + auto withWLockPtr(Function&& function) { + return function(wlock()); + } + template + auto withWLockPtr(Function&& function) const { + return function(wlock()); + } + + /** + * Invoke a function while holding an the lock in shared mode. + * + * A const reference to the datum will be passed into the function as its + * only argument. + */ + template + auto withRLock(Function&& function) const { + LockedGuardPtr guardPtr( + static_cast(this)); + return function(*guardPtr); + } + + template + auto withRLockPtr(Function&& function) const { + return function(rlock()); + } }; /** @@ -160,6 +231,57 @@ class SynchronizedBase { ConstLockedPtr lock(const std::chrono::duration& timeout) const { return ConstLockedPtr(static_cast(this), timeout); } + + /* + * Note: C++ 17 adds guaranteed copy elision. (http://wg21.link/P0135) + * Once compilers support this, it would be nice to add guard() methods that + * return LockedGuardPtr objects. + */ + + /** + * Invoke a function while holding the lock. + * + * A reference to the datum will be passed into the function as its only + * argument. + * + * This can be used with a lambda argument for easily defining small critical + * sections in the code. For example: + * + * auto value = obj.withLock([](auto& data) { + * data.doStuff(); + * return data.getValue(); + * }); + */ + template + auto withLock(Function&& function) { + LockedGuardPtr guardPtr( + static_cast(this)); + return function(*guardPtr); + } + template + auto withLock(Function&& function) const { + LockedGuardPtr guardPtr( + static_cast(this)); + return function(*guardPtr); + } + + /** + * Invoke a function while holding the lock exclusively. + * + * This is similar to withWLock(), but the function will be passed a + * LockedPtr rather than a reference to the data itself. + * + * This allows scopedUnlock() and getUniqueLock() to be called on the + * LockedPtr argument. + */ + template + auto withLockPtr(Function&& function) { + return function(lock()); + } + template + auto withLockPtr(Function&& function) const { + return function(lock()); + } }; /** @@ -457,6 +579,8 @@ struct Synchronized : public SynchronizedBase< friend class folly::LockedPtrBase; template friend class folly::LockedPtr; + template + friend class folly::LockedGuardPtr; /** * Helper constructors to enable Synchronized for @@ -481,6 +605,27 @@ struct Synchronized : public SynchronizedBase< template class ScopedUnlocker; +namespace detail { +/* + * A helper alias that resolves to "const T" if the template parameter + * is a const Synchronized, or "T" if the parameter is not const. + */ +template +using SynchronizedDataType = typename std::conditional< + std::is_const::value, + typename SynchronizedType::DataType const, + typename SynchronizedType::DataType>::type; +/* + * A helper alias that resolves to a ConstLockedPtr if the template parameter + * is a const Synchronized, or a LockedPtr if the parameter is not const. + */ +template +using LockedPtrType = typename std::conditional< + std::is_const::value, + typename SynchronizedType::ConstLockedPtr, + typename SynchronizedType::LockedPtr>::type; +} // detail + /** * A helper base class for implementing LockedPtr. * @@ -701,10 +846,7 @@ class LockedPtr : public LockedPtrBase< LockPolicy>; using UnlockerData = typename Base::UnlockerData; // CDataType is the DataType with the appropriate const-qualification - using CDataType = typename std::conditional< - std::is_const::value, - typename SynchronizedType::DataType const, - typename SynchronizedType::DataType>::type; + using CDataType = detail::SynchronizedDataType; public: using DataType = typename SynchronizedType::DataType; @@ -812,17 +954,61 @@ class LockedPtr : public LockedPtrBase< } }; -namespace detail { -/* - * A helper alias to select a ConstLockedPtr if the template parameter is a - * const Synchronized, or a LockedPtr if the parameter is not const. +/** + * LockedGuardPtr is a simplified version of LockedPtr. + * + * It is non-movable, and supports fewer features than LockedPtr. However, it + * is ever-so-slightly more performant than LockedPtr. (The destructor can + * unconditionally release the lock, without requiring a conditional branch.) + * + * The relationship between LockedGuardPtr and LockedPtr is similar to that + * between std::lock_guard and std::unique_lock. */ -template -using LockedPtrType = typename std::conditional< - std::is_const::value, - typename SynchronizedType::ConstLockedPtr, - typename SynchronizedType::LockedPtr>::type; -} // detail +template +class LockedGuardPtr { + private: + // CDataType is the DataType with the appropriate const-qualification + using CDataType = detail::SynchronizedDataType; + + public: + using DataType = typename SynchronizedType::DataType; + using MutexType = typename SynchronizedType::MutexType; + using Synchronized = typename std::remove_const::type; + + LockedGuardPtr() = delete; + + /** + * Takes a Synchronized and locks it. + */ + explicit LockedGuardPtr(SynchronizedType* parent) : parent_(parent) { + LockPolicy::lock(parent_->mutex_); + } + + /** + * Destructor releases. + */ + ~LockedGuardPtr() { + LockPolicy::unlock(parent_->mutex_); + } + + /** + * Access the locked data. + */ + CDataType* operator->() const { + return &parent_->datum_; + } + + /** + * Access the locked data. + */ + CDataType& operator*() const { + return parent_->datum_; + } + + private: + // This is the entire state of LockedGuardPtr. + SynchronizedType* const parent_{nullptr}; +}; /** * Acquire locks for multiple Synchronized objects, in a deadlock-safe diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp index 3c07f57e..de45a404 100644 --- a/folly/test/SynchronizedTest.cpp +++ b/folly/test/SynchronizedTest.cpp @@ -59,6 +59,10 @@ TYPED_TEST(SynchronizedTest, Basic) { testBasic(); } +TYPED_TEST(SynchronizedTest, WithLock) { + testWithLock(); +} + TYPED_TEST(SynchronizedTest, Deprecated) { testDeprecated(); } diff --git a/folly/test/SynchronizedTestLib-inl.h b/folly/test/SynchronizedTestLib-inl.h index 3166de8a..d4e027cf 100644 --- a/folly/test/SynchronizedTestLib-inl.h +++ b/folly/test/SynchronizedTestLib-inl.h @@ -108,6 +108,7 @@ template typename std::enable_if::is_shared>::type testBasicImpl() { folly::Synchronized, Mutex> obj; + const auto& constObj = obj; obj.wlock()->resize(1000); @@ -141,7 +142,6 @@ testBasicImpl() { obj.wlock()->front() = 2; { - const auto& constObj = obj; // contextualLock() on a const reference should grab a shared lock auto lockedObj = constObj.contextualLock(); EXPECT_EQ(2, lockedObj->front()); @@ -160,6 +160,7 @@ template typename std::enable_if::is_shared>::type testBasicImpl() { folly::Synchronized, Mutex> obj; + const auto& constObj = obj; obj.lock()->resize(1000); @@ -178,6 +179,12 @@ testBasicImpl() { EXPECT_EQ(1001, obj.lock()->size()); } } + { + auto lockedObj = constObj.lock(); + EXPECT_EQ(1001, lockedObj->size()); + EXPECT_EQ(10, lockedObj->back()); + EXPECT_EQ(1000, obj2.lock()->size()); + } obj.lock()->front() = 2; @@ -193,6 +200,160 @@ void testBasic() { testBasicImpl(); } +// testWithLock() version for shared lock types +template +typename std::enable_if::is_shared>::type +testWithLock() { + folly::Synchronized, Mutex> obj; + const auto& constObj = obj; + + // Test withWLock() and withRLock() + obj.withWLock([](std::vector& lockedObj) { + lockedObj.resize(1000); + lockedObj.push_back(10); + lockedObj.push_back(11); + }); + obj.withWLock([](const std::vector& lockedObj) { + EXPECT_EQ(1002, lockedObj.size()); + }); + constObj.withWLock([](const std::vector& lockedObj) { + EXPECT_EQ(1002, lockedObj.size()); + EXPECT_EQ(11, lockedObj.back()); + }); + obj.withRLock([](const std::vector& lockedObj) { + EXPECT_EQ(1002, lockedObj.size()); + EXPECT_EQ(11, lockedObj.back()); + }); + constObj.withRLock([](const std::vector& lockedObj) { + EXPECT_EQ(1002, lockedObj.size()); + }); + +#if __cpp_generic_lambdas >= 201304 + obj.withWLock([](auto& lockedObj) { lockedObj.push_back(12); }); + obj.withWLock( + [](const auto& lockedObj) { EXPECT_EQ(1003, lockedObj.size()); }); + constObj.withWLock([](const auto& lockedObj) { + EXPECT_EQ(1003, lockedObj.size()); + EXPECT_EQ(12, lockedObj.back()); + }); + obj.withRLock([](const auto& lockedObj) { + EXPECT_EQ(1003, lockedObj.size()); + EXPECT_EQ(12, lockedObj.back()); + }); + constObj.withRLock( + [](const auto& lockedObj) { EXPECT_EQ(1003, lockedObj.size()); }); + obj.withWLock([](auto& lockedObj) { lockedObj.pop_back(); }); +#endif + + // Test withWLockPtr() and withRLockPtr() + using SynchType = folly::Synchronized, Mutex>; +#if __cpp_generic_lambdas >= 201304 + obj.withWLockPtr([](auto&& lockedObj) { lockedObj->push_back(13); }); + obj.withRLockPtr([](auto&& lockedObj) { + EXPECT_EQ(1003, lockedObj->size()); + EXPECT_EQ(13, lockedObj->back()); + }); + constObj.withRLockPtr([](auto&& lockedObj) { + EXPECT_EQ(1003, lockedObj->size()); + EXPECT_EQ(13, lockedObj->back()); + }); + obj.withWLockPtr([&](auto&& lockedObj) { + lockedObj->push_back(14); + { + auto unlocker = lockedObj.scopedUnlock(); + obj.wlock()->push_back(15); + } + EXPECT_EQ(15, lockedObj->back()); + }); + constObj.withWLockPtr([](auto&& lockedObj) { + EXPECT_EQ(1005, lockedObj->size()); + EXPECT_EQ(15, lockedObj->back()); + }); +#else + obj.withWLockPtr([](typename SynchType::LockedPtr&& lockedObj) { + lockedObj->push_back(13); + lockedObj->push_back(14); + lockedObj->push_back(15); + }); +#endif + + obj.withWLockPtr([](typename SynchType::LockedPtr&& lockedObj) { + lockedObj->push_back(16); + EXPECT_EQ(1006, lockedObj->size()); + }); + constObj.withWLockPtr([](typename SynchType::ConstWLockedPtr&& lockedObj) { + EXPECT_EQ(1006, lockedObj->size()); + EXPECT_EQ(16, lockedObj->back()); + }); + obj.withRLockPtr([](typename SynchType::ConstLockedPtr&& lockedObj) { + EXPECT_EQ(1006, lockedObj->size()); + EXPECT_EQ(16, lockedObj->back()); + }); + constObj.withRLockPtr([](typename SynchType::ConstLockedPtr&& lockedObj) { + EXPECT_EQ(1006, lockedObj->size()); + EXPECT_EQ(16, lockedObj->back()); + }); +} + +// testWithLock() version for non-shared lock types +template +typename std::enable_if::is_shared>::type +testWithLock() { + folly::Synchronized, Mutex> obj; + + // Test withLock() + obj.withLock([](std::vector& lockedObj) { + lockedObj.resize(1000); + lockedObj.push_back(10); + lockedObj.push_back(11); + }); + obj.withLock([](const std::vector& lockedObj) { + EXPECT_EQ(1002, lockedObj.size()); + }); + +#if __cpp_generic_lambdas >= 201304 + obj.withLock([](auto& lockedObj) { lockedObj.push_back(12); }); + obj.withLock( + [](const auto& lockedObj) { EXPECT_EQ(1003, lockedObj.size()); }); + obj.withLock([](auto& lockedObj) { lockedObj.pop_back(); }); +#endif + + // Test withLockPtr() + using SynchType = folly::Synchronized, Mutex>; +#if __cpp_generic_lambdas >= 201304 + obj.withLockPtr([](auto&& lockedObj) { lockedObj->push_back(13); }); + obj.withLockPtr([](auto&& lockedObj) { + EXPECT_EQ(1003, lockedObj->size()); + EXPECT_EQ(13, lockedObj->back()); + }); + obj.withLockPtr([&](auto&& lockedObj) { + lockedObj->push_back(14); + { + auto unlocker = lockedObj.scopedUnlock(); + obj.lock()->push_back(15); + } + EXPECT_EQ(1005, lockedObj->size()); + EXPECT_EQ(15, lockedObj->back()); + }); +#else + obj.withLockPtr([](typename SynchType::LockedPtr&& lockedObj) { + lockedObj->push_back(13); + lockedObj->push_back(14); + lockedObj->push_back(15); + }); +#endif + + obj.withLockPtr([](typename SynchType::LockedPtr&& lockedObj) { + lockedObj->push_back(16); + EXPECT_EQ(1006, lockedObj->size()); + }); + const auto& constObj = obj; + constObj.withLockPtr([](typename SynchType::ConstLockedPtr&& lockedObj) { + EXPECT_EQ(1006, lockedObj->size()); + EXPECT_EQ(16, lockedObj->back()); + }); +} + // Testing the deprecated SYNCHRONIZED and SYNCHRONIZED_CONST APIs template void testDeprecated() { -- 2.34.1