diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2023-06-08 12:08:52 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2023-06-08 12:08:52 +0000 |
commit | 341ffc2689341fa7f2116b5a9b304e11d8283ad0 (patch) | |
tree | aa5227922843ffb0df12016ed5217d003e950cb7 | |
parent | 96c08c994dfaf4d605878e3e4f4ac982937b30bc (diff) |
Avoid allocating LockKeeper on heap, just move it along with self.
-rw-r--r-- | storage/src/tests/bucketdb/lockablemaptest.cpp | 2 | ||||
-rw-r--r-- | storage/src/vespa/storage/bucketdb/abstract_bucket_map.h | 76 |
2 files changed, 53 insertions, 25 deletions
diff --git a/storage/src/tests/bucketdb/lockablemaptest.cpp b/storage/src/tests/bucketdb/lockablemaptest.cpp index ec3567ad795..2ac670740c0 100644 --- a/storage/src/tests/bucketdb/lockablemaptest.cpp +++ b/storage/src/tests/bucketdb/lockablemaptest.cpp @@ -781,7 +781,7 @@ TYPED_TEST(LockableMapTest, entry_changes_not_visible_if_write_not_invoked_on_gu TYPED_TEST(LockableMapTest, track_sizes) { TypeParam map; - EXPECT_EQ(40ul, sizeof(typename TypeParam::WrappedEntry)); + EXPECT_EQ(56ul, sizeof(typename TypeParam::WrappedEntry)); } } // storage diff --git a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h index 7b0ee95210d..1004a848c5e 100644 --- a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h +++ b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h @@ -11,6 +11,7 @@ #include <functional> #include <iosfwd> #include <map> +#include <optional> namespace storage::bucketdb { @@ -31,15 +32,42 @@ public: private: // Responsible for releasing lock in map when out of scope. class LockKeeper { - friend struct WrappedEntry; - AbstractBucketMap& _map; - key_type _key; - bool _locked; - void unlock() { _map.unlock(_key); _locked = false; } public: + LockKeeper() noexcept + : _map(nullptr), _key(), _locked(false) {} LockKeeper(AbstractBucketMap& map, key_type key) noexcept - : _map(map), _key(key), _locked(true) {} - ~LockKeeper() { if (_locked) unlock(); } + : _map(&map), _key(key), _locked(true) {} + LockKeeper(LockKeeper && rhs) noexcept + : _map(rhs._map), + _key(rhs._key), + _locked(rhs._locked) + { + rhs._locked = false; + } + LockKeeper & operator=(LockKeeper && rhs) noexcept { + if (&rhs == this) return *this; + cleanup(); + _map = rhs._map; + _key = rhs._key; + _locked = rhs._locked; + rhs._locked = false; + return *this; + } + ~LockKeeper() { cleanup(); } + AbstractBucketMap & map() { return *_map; } + void unlock() { + _map->unlock(_key); + _locked = false; + } + bool locked() const noexcept { return _locked; } + const key_type & key() const noexcept { return _key; } + private: + void cleanup() { + if (_locked) unlock(); + } + AbstractBucketMap * _map; + key_type _key; + bool _locked; }; public: @@ -52,14 +80,14 @@ public: _preExisted(false) {} WrappedEntry(AbstractBucketMap& map, const key_type& key, const mapped_type& val, - const char* clientId, bool preExisted_) - : _lockKeeper(std::make_unique<LockKeeper>(map, key)), + const char* clientId, bool preExisted_) noexcept + : _lockKeeper(map, key), _value(val), _clientId(clientId), _exists(true), _preExisted(preExisted_) {} - WrappedEntry(AbstractBucketMap& map, const key_type& key, const char* clientId) - : _lockKeeper(std::make_unique<LockKeeper>(map, key)), + WrappedEntry(AbstractBucketMap& map, const key_type& key, const char* clientId) noexcept + : _lockKeeper(map, key), _value(), _clientId(clientId), _exists(false), @@ -82,18 +110,18 @@ public: void unlock(); [[nodiscard]] bool exist() const { return _exists; } // TODO rename to exists() [[nodiscard]] bool preExisted() const { return _preExisted; } - [[nodiscard]] bool locked() const { return _lockKeeper.get(); } - const key_type& getKey() const { return _lockKeeper->_key; }; + [[nodiscard]] bool locked() const { return _lockKeeper.locked(); } + const key_type& getKey() const { return _lockKeeper.key(); }; BucketId getBucketId() const { return BucketId(BucketId::keyToBucketId(getKey())); } protected: - std::unique_ptr<LockKeeper> _lockKeeper; + LockKeeper _lockKeeper; mapped_type _value; const char* _clientId; - bool _exists; - bool _preExisted; + bool _exists; + bool _preExisted; }; struct LockId { @@ -214,25 +242,25 @@ AbstractBucketMap<ValueT>::WrappedEntry::~WrappedEntry() = default; template <typename ValueT> void AbstractBucketMap<ValueT>::WrappedEntry::write() { - assert(_lockKeeper->_locked); + assert(_lockKeeper.locked()); assert(_value.verifyLegal()); bool b; - _lockKeeper->_map.insert(_lockKeeper->_key, _value, _clientId, true, b); - _lockKeeper->unlock(); + _lockKeeper.map().insert(_lockKeeper.key(), _value, _clientId, true, b); + _lockKeeper.unlock(); } template <typename ValueT> void AbstractBucketMap<ValueT>::WrappedEntry::remove() { - assert(_lockKeeper->_locked); + assert(_lockKeeper.locked()); assert(_exists); - _lockKeeper->_map.erase(_lockKeeper->_key, _clientId, true); - _lockKeeper->unlock(); + _lockKeeper.map().erase(_lockKeeper.key(), _clientId, true); + _lockKeeper.unlock(); } template <typename ValueT> void AbstractBucketMap<ValueT>::WrappedEntry::unlock() { - assert(_lockKeeper->_locked); - _lockKeeper->unlock(); + assert(_lockKeeper.locked()); + _lockKeeper.unlock(); } } |