diff options
author | Tor Brede Vekterli <vekterli@verizonmedia.com> | 2020-07-06 13:02:04 +0000 |
---|---|---|
committer | Tor Brede Vekterli <vekterli@verizonmedia.com> | 2020-07-07 12:33:44 +0000 |
commit | b87926e071aa8603a5856b1d94caf77f571875c2 (patch) | |
tree | c427180d082c2ceef14bd8e804c7b98934c5742e /storage | |
parent | 475bd34f0c56bb6d59ae6f5bb7c12092dc6819f4 (diff) |
Expose ReadGuard via AbstractLockableMap interface
* Add working B-tree snapshot read guard impl
* Add placeholder wrapper read guard for legacy DB
* Enforce value const-ness of existing for_each_chunked iteration API
* Return read guard entries by value instead of modifying ref argument
Diffstat (limited to 'storage')
16 files changed, 281 insertions, 78 deletions
diff --git a/storage/src/tests/bucketdb/lockablemaptest.cpp b/storage/src/tests/bucketdb/lockablemaptest.cpp index c5d22b6e6b5..c0d29aad158 100644 --- a/storage/src/tests/bucketdb/lockablemaptest.cpp +++ b/storage/src/tests/bucketdb/lockablemaptest.cpp @@ -6,6 +6,7 @@ #include <vespa/storage/bucketdb/lockablemap.hpp> #include <vespa/storage/bucketdb/btree_lockable_map.hpp> #include <vespa/vespalib/gtest/gtest.h> +#include <gmock/gmock.h> #include <vespa/log/log.h> LOG_SETUP(".lockable_map_test"); @@ -200,13 +201,23 @@ EntryProcessor<Map>::~EntryProcessor() = default; template <typename Map> struct ConstProcessor { + mutable uint32_t count; mutable std::vector<std::string> log; + mutable std::vector<typename Map::Decision> behaviour; + + ConstProcessor(); + explicit ConstProcessor(std::vector<typename Map::Decision> decisions); + ~ConstProcessor(); typename Map::Decision operator()(uint64_t key, const A& a) const { std::ostringstream ost; ost << key << " - " << a; log.push_back(ost.str()); - return Map::CONTINUE; + auto d = Map::Decision::CONTINUE; + if (behaviour.size() > count) { + d = behaviour[count++]; + } + return d; } std::string toString() { @@ -218,6 +229,17 @@ struct ConstProcessor { } }; +template <typename Map> +ConstProcessor<Map>::ConstProcessor() + : count(0), log(), behaviour() {} + +template <typename Map> +ConstProcessor<Map>::ConstProcessor(std::vector<typename Map::Decision> decisions) + : count(0), log(), behaviour(std::move(decisions)) {} + +template <typename Map> +ConstProcessor<Map>::~ConstProcessor() = default; + } TYPED_TEST(LockableMapTest, iterating) { @@ -300,17 +322,21 @@ TYPED_TEST(LockableMapTest, chunked_iteration_is_transparent_across_chunk_sizes) map.insert(16, A(1, 2, 3), "foo", preExisted); map.insert(11, A(4, 6, 0), "foo", preExisted); map.insert(14, A(42, 0, 0), "foo", preExisted); - NonConstProcessor<TypeParam> ncproc; // Increments 2nd value in all entries. - // for_each_chunked with chunk size of 1 - map.for_each_chunked(std::ref(ncproc), "foo", 1us, 1); - EXPECT_EQ(A(4, 7, 0), *map.get(11, "foo")); - EXPECT_EQ(A(42, 1, 0), *map.get(14, "foo")); - EXPECT_EQ(A(1, 3, 3), *map.get(16, "foo")); - // for_each_chunked with chunk size larger than db size - map.for_each_chunked(std::ref(ncproc), "foo", 1us, 100); - EXPECT_EQ(A(4, 8, 0), *map.get(11, "foo")); - EXPECT_EQ(A(42, 2, 0), *map.get(14, "foo")); - EXPECT_EQ(A(1, 4, 3), *map.get(16, "foo")); + std::string expected("11 - A(4, 6, 0)\n" + "14 - A(42, 0, 0)\n" + "16 - A(1, 2, 3)\n"); + { + ConstProcessor<TypeParam> cproc; + // for_each_chunked with chunk size of 1 + map.for_each_chunked(std::ref(cproc), "foo", 1us, 1); + EXPECT_EQ(expected, cproc.toString()); + } + { + ConstProcessor<TypeParam> cproc; + // for_each_chunked with chunk size larger than db size + map.for_each_chunked(std::ref(cproc), "foo", 1us, 100); + EXPECT_EQ(expected, cproc.toString()); + } } TYPED_TEST(LockableMapTest, can_abort_during_chunked_iteration) { @@ -323,10 +349,10 @@ TYPED_TEST(LockableMapTest, can_abort_during_chunked_iteration) { std::vector<typename TypeParam::Decision> decisions; decisions.push_back(TypeParam::CONTINUE); decisions.push_back(TypeParam::ABORT); - EntryProcessor<TypeParam> proc(decisions); + ConstProcessor<TypeParam> proc(std::move(decisions)); map.for_each_chunked(std::ref(proc), "foo", 1us, 100); std::string expected("11 - A(4, 6, 0)\n" - "14 - A(42, 0, 0)\n"); + "14 - A(42, 0, 0)\n"); EXPECT_EQ(expected, proc.toString()); } @@ -509,6 +535,18 @@ TYPED_TEST(LockableMapTest, find_all) { EXPECT_EQ(1, results.size()); EXPECT_EQ(A(9,10,11), *results[id9.stripUnused()]); // sub bucket + + // Make sure we clear any existing bucket locks before we continue, or test will deadlock + // if running with legacy (non-snapshot capable) DB implementation. + results.clear(); + // Results should be equal when using read guard + auto guard = map.acquire_read_guard(); + + auto guard_results = guard->find_parents_self_and_children(BucketId(17, 0x1aaaa)); + EXPECT_THAT(guard_results, ElementsAre(A(1,2,3), A(5,6,7), A(6,7,8), A(7,8,9))); + + guard_results = guard->find_parents_self_and_children(BucketId(16, 0xffff)); + EXPECT_THAT(guard_results, ElementsAre(A(9,10,11))); } TYPED_TEST(LockableMapTest, find_all_2) { // Ticket 3121525 diff --git a/storage/src/tests/distributor/btree_bucket_database_test.cpp b/storage/src/tests/distributor/btree_bucket_database_test.cpp index a2518272a7f..5283cc39da8 100644 --- a/storage/src/tests/distributor/btree_bucket_database_test.cpp +++ b/storage/src/tests/distributor/btree_bucket_database_test.cpp @@ -37,8 +37,10 @@ struct BTreeReadGuardTest : Test { TEST_F(BTreeReadGuardTest, guard_does_not_observe_new_entries) { auto guard = _db.acquire_read_guard(); _db.update(BucketDatabase::Entry(BucketId(16, 16), BI(1, 1234))); - std::vector<BucketDatabase::Entry> entries; - guard->find_parents_and_self(BucketId(16, 16), entries); + + auto entries = guard->find_parents_and_self(BucketId(16, 16)); + EXPECT_EQ(entries.size(), 0U); + entries = guard->find_parents_self_and_children(BucketId(16, 16)); EXPECT_EQ(entries.size(), 0U); } @@ -47,8 +49,12 @@ TEST_F(BTreeReadGuardTest, guard_observes_entries_alive_at_acquire_time) { _db.update(BucketDatabase::Entry(bucket, BI(1, 1234))); auto guard = _db.acquire_read_guard(); _db.remove(bucket); - std::vector<BucketDatabase::Entry> entries; - guard->find_parents_and_self(bucket, entries); + + auto entries = guard->find_parents_and_self(bucket); + ASSERT_EQ(entries.size(), 1U); + EXPECT_EQ(entries[0].getBucketInfo(), BI(1, 1234)); + + entries = guard->find_parents_self_and_children(bucket); ASSERT_EQ(entries.size(), 1U); EXPECT_EQ(entries[0].getBucketInfo(), BI(1, 1234)); } diff --git a/storage/src/tests/distributor/bucketdatabasetest.cpp b/storage/src/tests/distributor/bucketdatabasetest.cpp index 0b832699364..7f9825d32fc 100644 --- a/storage/src/tests/distributor/bucketdatabasetest.cpp +++ b/storage/src/tests/distributor/bucketdatabasetest.cpp @@ -173,8 +173,7 @@ BucketDatabaseTest::doFindParents(const std::vector<document::BucketId>& ids, // TODO remove in favor of only read guard once legacy DB usage has been ported over db().getParents(searchId, entries); - std::vector<BucketDatabase::Entry> checked_entries; - db().acquire_read_guard()->find_parents_and_self(searchId, checked_entries); + auto checked_entries = db().acquire_read_guard()->find_parents_and_self(searchId); if (entries != checked_entries) { return "Mismatch between results from getParents() and ReadGuard!"; } diff --git a/storage/src/tests/distributor/bucketdbupdatertest.cpp b/storage/src/tests/distributor/bucketdbupdatertest.cpp index 495b6fb570e..4b8ea56e5ca 100644 --- a/storage/src/tests/distributor/bucketdbupdatertest.cpp +++ b/storage/src/tests/distributor/bucketdbupdatertest.cpp @@ -2769,8 +2769,7 @@ struct BucketDBUpdaterSnapshotTest : BucketDBUpdaterTest { uint32_t found_buckets = 0; for_each_bucket(repo, [&](const auto& space, const auto& entry) { if (space == bucket_space) { - std::vector<BucketDatabase::Entry> entries; - guard->find_parents_and_self(entry.getBucketId(), entries); + auto entries = guard->find_parents_and_self(entry.getBucketId()); if (entries.size() == 1) { ++found_buckets; } diff --git a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h index a6855532e30..ae4c48ed22f 100644 --- a/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h +++ b/storage/src/vespa/storage/bucketdb/abstract_bucket_map.h @@ -1,6 +1,7 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include "read_guard.h" #include <vespa/document/bucket/bucketid.h> #include <vespa/vespalib/stllike/hash_map.h> #include <vespa/vespalib/stllike/hash_set.h> @@ -161,7 +162,7 @@ public: * * Type erasure of functor needed due to virtual indirection. */ - void for_each_chunked(std::function<Decision(uint64_t, ValueT&)> func, + void for_each_chunked(std::function<Decision(uint64_t, const ValueT&)> func, const char* clientId, vespalib::duration yieldTime = 10us, uint32_t chunkSize = DEFAULT_CHUNK_SIZE) @@ -185,6 +186,10 @@ public: do_for_each(std::move(func), clientId, first, last); } + std::unique_ptr<bucketdb::ReadGuard<ValueT>> acquire_read_guard() const { + return do_acquire_read_guard(); + } + [[nodiscard]] virtual size_type size() const noexcept = 0; [[nodiscard]] virtual size_type getMemoryUsage() const noexcept = 0; [[nodiscard]] virtual bool empty() const noexcept = 0; @@ -194,7 +199,7 @@ public: virtual void print(std::ostream& out, bool verbose, const std::string& indent) const = 0; private: virtual void unlock(const key_type& key) = 0; // Only for bucket lock guards - virtual void do_for_each_chunked(std::function<Decision(uint64_t, ValueT&)> func, + virtual void do_for_each_chunked(std::function<Decision(uint64_t, const ValueT&)> func, const char* clientId, vespalib::duration yieldTime, uint32_t chunkSize) = 0; @@ -206,6 +211,7 @@ private: const char* clientId, const key_type& first, const key_type& last) = 0; + virtual std::unique_ptr<bucketdb::ReadGuard<ValueT>> do_acquire_read_guard() const = 0; }; template <typename ValueT> diff --git a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp index 9634d6d0953..7bf78c8ba7e 100644 --- a/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp +++ b/storage/src/vespa/storage/bucketdb/btree_bucket_database.cpp @@ -198,8 +198,9 @@ public: explicit ReadGuardImpl(const BTreeBucketDatabase& db); ~ReadGuardImpl() override; - void find_parents_and_self(const document::BucketId& bucket, - std::vector<Entry>& entries) const override; + std::vector<Entry> find_parents_and_self(const document::BucketId& bucket) const override; + std::vector<Entry> find_parents_self_and_children(const document::BucketId& bucket) const override; + void for_each(std::function<void(uint64_t, const Entry&)> func) const override; [[nodiscard]] uint64_t generation() const noexcept override; }; @@ -209,12 +210,26 @@ BTreeBucketDatabase::ReadGuardImpl::ReadGuardImpl(const BTreeBucketDatabase& db) BTreeBucketDatabase::ReadGuardImpl::~ReadGuardImpl() = default; -void BTreeBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket, - std::vector<Entry>& entries) const -{ +std::vector<Entry> +BTreeBucketDatabase::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket) const { + std::vector<Entry> entries; _snapshot.find_parents_and_self<ByValue>(bucket, [&entries]([[maybe_unused]] uint64_t key, Entry entry){ entries.emplace_back(std::move(entry)); }); + return entries; +} + +std::vector<Entry> +BTreeBucketDatabase::ReadGuardImpl::find_parents_self_and_children(const document::BucketId& bucket) const { + std::vector<Entry> entries; + _snapshot.find_parents_self_and_children<ByValue>(bucket, [&entries]([[maybe_unused]] uint64_t key, Entry entry){ + entries.emplace_back(std::move(entry)); + }); + return entries; +} + +void BTreeBucketDatabase::ReadGuardImpl::for_each(std::function<void(uint64_t, const Entry&)> func) const { + _snapshot.for_each<ByValue>(std::move(func)); } uint64_t BTreeBucketDatabase::ReadGuardImpl::generation() const noexcept { diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h index 3d58b85b063..ba2e6ca28a0 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.h +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.h @@ -101,11 +101,14 @@ private: WaiterMap _map; }; - mutable std::mutex _lock; - std::condition_variable _cond; - std::unique_ptr<GenericBTreeBucketDatabase<ValueTraits>> _impl; - LockIdSet _lockedKeys; - LockWaiters _lockWaiters; + class ReadGuardImpl; + using ImplType = GenericBTreeBucketDatabase<ValueTraits>; + + mutable std::mutex _lock; + std::condition_variable _cond; + std::unique_ptr<ImplType> _impl; + LockIdSet _lockedKeys; + LockWaiters _lockWaiters; void unlock(const key_type& key) override; bool findNextKey(key_type& key, mapped_type& val, const char* clientId, @@ -123,11 +126,13 @@ private: const key_type& first, const key_type& last) override; - void do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func, + void do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func, const char* client_id, vespalib::duration yield_time, uint32_t chunk_size) override; + std::unique_ptr<ReadGuard<T>> do_acquire_read_guard() const override; + /** * Process up to `chunk_size` bucket database entries from--and possibly * including--the bucket pointed to by `key`. @@ -139,7 +144,7 @@ private: * Modifies `key` in-place to point to the next key to process for the next * invocation of this function. */ - bool processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func, + bool processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func, key_type& key, const char* client_id, uint32_t chunk_size); diff --git a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp index 18eae405dc6..e8c91f04a9e 100644 --- a/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp +++ b/storage/src/vespa/storage/bucketdb/btree_lockable_map.hpp @@ -307,7 +307,7 @@ void BTreeLockableMap<T>::do_for_each(std::function<Decision(uint64_t, const map } template <typename T> -bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func, +bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func, key_type& key, const char* client_id, const uint32_t chunk_size) @@ -328,7 +328,7 @@ bool BTreeLockableMap<T>::processNextChunk(std::function<Decision(uint64_t, mapp } template <typename T> -void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func, +void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func, const char* client_id, vespalib::duration yield_time, uint32_t chunk_size) @@ -347,6 +347,66 @@ void BTreeLockableMap<T>::do_for_each_chunked(std::function<Decision(uint64_t, m } template <typename T> +class BTreeLockableMap<T>::ReadGuardImpl final : public bucketdb::ReadGuard<T> { + typename ImplType::ReadSnapshot _snapshot; +public: + explicit ReadGuardImpl(const BTreeLockableMap<T>& db); + ~ReadGuardImpl() override; + + std::vector<T> find_parents_and_self(const document::BucketId& bucket) const override; + std::vector<T> find_parents_self_and_children(const document::BucketId& bucket) const override; + void for_each(std::function<void(uint64_t, const T&)> func) const override; + [[nodiscard]] uint64_t generation() const noexcept override; +}; + +template <typename T> +BTreeLockableMap<T>::ReadGuardImpl::ReadGuardImpl(const BTreeLockableMap<T>& db) + : _snapshot(*db._impl) +{} + +template <typename T> +BTreeLockableMap<T>::ReadGuardImpl::~ReadGuardImpl() = default; + +template <typename T> +std::vector<T> +BTreeLockableMap<T>::ReadGuardImpl::find_parents_and_self(const document::BucketId& bucket) const { + std::vector<T> entries; + _snapshot.template find_parents_and_self<ByConstRef>( + bucket, + [&entries]([[maybe_unused]] uint64_t key, const T& entry){ + entries.emplace_back(entry); + }); + return entries; +} + +template <typename T> +std::vector<T> +BTreeLockableMap<T>::ReadGuardImpl::find_parents_self_and_children(const document::BucketId& bucket) const { + std::vector<T> entries; + _snapshot.template find_parents_self_and_children<ByConstRef>( + bucket, + [&entries]([[maybe_unused]] uint64_t key, const T& entry){ + entries.emplace_back(entry); + }); + return entries; +} + +template <typename T> +void BTreeLockableMap<T>::ReadGuardImpl::for_each(std::function<void(uint64_t, const T&)> func) const { + _snapshot.template for_each<ByConstRef>(std::move(func)); +} + +template <typename T> +uint64_t BTreeLockableMap<T>::ReadGuardImpl::generation() const noexcept { + return _snapshot.generation(); +} + +template <typename T> +std::unique_ptr<ReadGuard<T>> BTreeLockableMap<T>::do_acquire_read_guard() const { + return std::make_unique<ReadGuardImpl>(*this); +} + +template <typename T> void BTreeLockableMap<T>::print(std::ostream& out, bool verbose, const std::string& indent) const { diff --git a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h index 8bc7a3379b3..977aeb8f925 100644 --- a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h +++ b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.h @@ -71,13 +71,6 @@ public: GenericBTreeBucketDatabase(GenericBTreeBucketDatabase&&) = delete; GenericBTreeBucketDatabase& operator=(GenericBTreeBucketDatabase&&) = delete; - // TODO move - struct EntryProcessor { - virtual ~EntryProcessor() = default; - /** Return false to stop iterating. */ - virtual bool process(const typename DataStoreTraitsT::ConstValueRef& e) = 0; - }; - ValueType entry_from_iterator(const BTreeConstIterator& iter) const; ConstValueRef const_value_ref_from_valid_iterator(const BTreeConstIterator& iter) const; @@ -103,14 +96,10 @@ public: bool update_by_raw_key(uint64_t bucket_key, const ValueType& new_entry); template <typename IterValueExtractor, typename Func> - void find_parents_and_self(const document::BucketId& bucket, - Func func) const; + void find_parents_and_self(const document::BucketId& bucket, Func func) const; template <typename IterValueExtractor, typename Func> - void find_parents_self_and_children(const document::BucketId& bucket, - Func func) const; - - void for_each(EntryProcessor& proc, const document::BucketId& after) const; + void find_parents_self_and_children(const document::BucketId& bucket, Func func) const; document::BucketId getAppropriateBucket(uint16_t minBits, const document::BucketId& bid) const; @@ -136,6 +125,10 @@ public: template <typename IterValueExtractor, typename Func> void find_parents_and_self(const document::BucketId& bucket, Func func) const; + template <typename IterValueExtractor, typename Func> + void find_parents_self_and_children(const document::BucketId& bucket, Func func) const; + template <typename IterValueExtractor, typename Func> + void for_each(Func func) const; [[nodiscard]] uint64_t generation() const noexcept; }; private: @@ -148,6 +141,11 @@ private: void find_parents_and_self_internal(const typename BTree::FrozenView& frozen_view, const document::BucketId& bucket, Func func) const; + template <typename IterValueExtractor, typename Func> + void find_parents_self_and_children_internal(const typename BTree::FrozenView& frozen_view, + const document::BucketId& bucket, + Func func) const; + void commit_tree_changes(); template <typename DataStoreTraitsT2> friend struct BTreeBuilderMerger; diff --git a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp index adaf402e4d1..5fb673a1440 100644 --- a/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp +++ b/storage/src/vespa/storage/bucketdb/generic_btree_bucket_database.hpp @@ -250,12 +250,12 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_and_self( template <typename DataStoreTraitsT> template <typename IterValueExtractor, typename Func> -void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children( +void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children_internal( + const typename BTree::FrozenView& frozen_view, const BucketId& bucket, Func func) const { - auto view = _tree.getFrozenView(); - auto iter = find_parents_internal<IterValueExtractor>(view, bucket, func); + auto iter = find_parents_internal<IterValueExtractor>(frozen_view, bucket, func); // `iter` is already pointing at, or beyond, one of the bucket's subtrees. for (; iter.valid(); ++iter) { auto candidate = BucketId(BucketId::keyToBucketId(iter.getKey())); @@ -268,6 +268,16 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_childre } template <typename DataStoreTraitsT> +template <typename IterValueExtractor, typename Func> +void GenericBTreeBucketDatabase<DataStoreTraitsT>::find_parents_self_and_children( + const BucketId& bucket, + Func func) const +{ + auto view = _tree.getFrozenView(); + find_parents_self_and_children_internal<IterValueExtractor>(view, bucket, std::move(func)); +} + +template <typename DataStoreTraitsT> typename GenericBTreeBucketDatabase<DataStoreTraitsT>::ValueType GenericBTreeBucketDatabase<DataStoreTraitsT>::get(const BucketId& bucket) const { return entry_from_iterator(_tree.find(bucket.toKey())); @@ -323,19 +333,6 @@ bool GenericBTreeBucketDatabase<DataStoreTraitsT>::update(const BucketId& bucket return update_by_raw_key(bucket.toKey(), new_entry); } -// TODO need snapshot read with guarding -// FIXME semantics of for-each in judy and bit tree DBs differ, former expects lbound, latter ubound..! -// FIXME but bit-tree code says "lowerBound" in impl and "after" in declaration??? -template <typename DataStoreTraitsT> -void GenericBTreeBucketDatabase<DataStoreTraitsT>::for_each(EntryProcessor& proc, const BucketId& after) const { - for (auto iter = _tree.upperBound(after.toKey()); iter.valid(); ++iter) { - // TODO memory fencing once we use snapshots! - if (!proc.process(DataStoreTraitsT::unwrap_const_ref_from_key_value(_store, iter.getKey(), iter.getData()))) { - break; - } - } -} - /* * Returns the bucket ID which, based on the buckets already existing in the DB, * is the most specific location in the tree in which it should reside. This may @@ -541,6 +538,24 @@ void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::find_parents_an } template <typename DataStoreTraitsT> +template <typename IterValueExtractor, typename Func> +void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::find_parents_self_and_children( + const BucketId& bucket, + Func func) const +{ + _db->find_parents_self_and_children_internal<IterValueExtractor>(_frozen_view, bucket, std::move(func)); +} + +template <typename DataStoreTraitsT> +template <typename IterValueExtractor, typename Func> +void GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::for_each(Func func) const { + for (auto iter = _frozen_view.begin(); iter.valid(); ++iter) { + // Iterator value extractor implicitly inserts any required memory fences for value. + func(iter.getKey(), IterValueExtractor::apply(*_db, iter)); + } +} + +template <typename DataStoreTraitsT> uint64_t GenericBTreeBucketDatabase<DataStoreTraitsT>::ReadSnapshot::generation() const noexcept { return _guard.getGeneration(); } diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.h b/storage/src/vespa/storage/bucketdb/lockablemap.h index 66619a4f7e8..a79a42b44ab 100644 --- a/storage/src/vespa/storage/bucketdb/lockablemap.h +++ b/storage/src/vespa/storage/bucketdb/lockablemap.h @@ -124,11 +124,13 @@ private: WaiterMap _map; }; - Map _map; + class ReadGuardImpl; + + Map _map; mutable std::mutex _lock; std::condition_variable _cond; - LockIdSet _lockedKeys; - LockWaiters _lockWaiters; + LockIdSet _lockedKeys; + LockWaiters _lockWaiters; void unlock(const key_type& key) override; bool findNextKey(key_type& key, mapped_type& val, const char* clientId, @@ -146,11 +148,13 @@ private: const key_type& first, const key_type& last) override; - void do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func, + void do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func, const char* clientId, vespalib::duration yieldTime, uint32_t chunkSize) override; + std::unique_ptr<bucketdb::ReadGuard<typename Map::mapped_type>> do_acquire_read_guard() const override; + /** * Process up to `chunkSize` bucket database entries from--and possibly * including--the bucket pointed to by `key`. @@ -162,7 +166,7 @@ private: * Modifies `key` in-place to point to the next key to process for the next * invocation of this function. */ - bool processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func, + bool processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func, key_type& key, const char* clientId, uint32_t chunkSize); diff --git a/storage/src/vespa/storage/bucketdb/lockablemap.hpp b/storage/src/vespa/storage/bucketdb/lockablemap.hpp index fcdde0a810c..c475361bd6f 100644 --- a/storage/src/vespa/storage/bucketdb/lockablemap.hpp +++ b/storage/src/vespa/storage/bucketdb/lockablemap.hpp @@ -247,7 +247,7 @@ void LockableMap<Map>::do_for_each(std::function<Decision(uint64_t, const mapped template <typename Map> bool -LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, mapped_type&)>& func, +LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, const mapped_type&)>& func, key_type& key, const char* clientId, const uint32_t chunkSize) @@ -268,7 +268,7 @@ LockableMap<Map>::processNextChunk(std::function<Decision(uint64_t, mapped_type& } template <typename Map> -void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, mapped_type&)> func, +void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, const mapped_type&)> func, const char* clientId, vespalib::duration yieldTime, uint32_t chunkSize) @@ -286,6 +286,55 @@ void LockableMap<Map>::do_for_each_chunked(std::function<Decision(uint64_t, mapp } } +// TODO This is a placeholder that has to work around the const-ness and type quirks of +// the legacy LockableMap implementation. In particular, it offers no snapshot isolation +// at all, nor does it support the "get parents and self" bucket lookup operation. +template <typename Map> +class LockableMap<Map>::ReadGuardImpl final + : public bucketdb::ReadGuard<typename Map::mapped_type> +{ + const LockableMap<Map>& _map; +public: + using mapped_type = typename Map::mapped_type; + + explicit ReadGuardImpl(const LockableMap<Map>& map) : _map(map) {} + ~ReadGuardImpl() override = default; + + std::vector<mapped_type> find_parents_and_self(const document::BucketId&) const override { + abort(); // Finding just parents+self isn't supported by underlying legacy DB API! + } + + std::vector<mapped_type> find_parents_self_and_children(const document::BucketId& bucket) const override { + auto& mutable_map = const_cast<LockableMap<Map>&>(_map); // _map is thread safe. + auto locked_entries = mutable_map.getAll(bucket, "ReadGuardImpl::find_parents_self_and_children"); + std::vector<mapped_type> entries; + entries.reserve(locked_entries.size()); + for (auto& e : locked_entries) { + entries.emplace_back(*e.second); + } + return entries; + } + + void for_each(std::function<void(uint64_t, const mapped_type&)> func) const override { + auto decision_wrapper = [&func](uint64_t key, const mapped_type& value) -> Decision { + func(key, value); + return Decision::CONTINUE; + }; + auto& mutable_map = const_cast<LockableMap<Map>&>(_map); // _map is thread safe. + mutable_map.for_each_chunked(std::move(decision_wrapper), "ReadGuardImpl::for_each"); + } + + [[nodiscard]] uint64_t generation() const noexcept override { + return 0; + } +}; + +template <typename Map> +std::unique_ptr<bucketdb::ReadGuard<typename Map::mapped_type>> +LockableMap<Map>::do_acquire_read_guard() const { + return std::make_unique<ReadGuardImpl>(*this); +} + template<typename Map> void LockableMap<Map>::print(std::ostream& out, bool verbose, diff --git a/storage/src/vespa/storage/bucketdb/read_guard.h b/storage/src/vespa/storage/bucketdb/read_guard.h index cf37bafb0dd..bacb0a38e2f 100644 --- a/storage/src/vespa/storage/bucketdb/read_guard.h +++ b/storage/src/vespa/storage/bucketdb/read_guard.h @@ -2,6 +2,7 @@ #pragma once #include <vespa/document/bucket/bucketid.h> +#include <functional> #include <vector> namespace storage::bucketdb { @@ -36,8 +37,9 @@ public: ReadGuard(const ReadGuard&) = delete; ReadGuard& operator=(const ReadGuard&) = delete; - virtual void find_parents_and_self(const document::BucketId& bucket, - std::vector<ValueT>& entries) const = 0; + virtual std::vector<ValueT> find_parents_and_self(const document::BucketId& bucket) const = 0; + virtual std::vector<ValueT> find_parents_self_and_children(const document::BucketId& bucket) const = 0; + virtual void for_each(std::function<void(uint64_t, const ValueT&)> func) const = 0; // If the underlying guard represents a snapshot, returns its monotonically // increasing generation. Otherwise returns 0. [[nodiscard]] virtual uint64_t generation() const noexcept = 0; diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp index 7066ea115fd..6940c072ed6 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.cpp +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.cpp @@ -136,6 +136,11 @@ void StorBucketDatabase::for_each( _impl->for_each(std::move(func), clientId, first, last); } +std::unique_ptr<bucketdb::ReadGuard<StorBucketDatabase::Entry>> +StorBucketDatabase::acquire_read_guard() const { + return {}; +} + template class JudyMultiMap<bucketdb::StorageBucketInfo>; } // storage diff --git a/storage/src/vespa/storage/bucketdb/storbucketdb.h b/storage/src/vespa/storage/bucketdb/storbucketdb.h index 87e5d80c01b..5aac27bccb0 100644 --- a/storage/src/vespa/storage/bucketdb/storbucketdb.h +++ b/storage/src/vespa/storage/bucketdb/storbucketdb.h @@ -2,6 +2,7 @@ #pragma once #include "abstract_bucket_map.h" +#include "read_guard.h" #include "storagebucketinfo.h" #include <vespa/storageapi/defs.h> #include <memory> @@ -68,6 +69,8 @@ public: const key_type& first = key_type(), const key_type& last = key_type() - 1); + std::unique_ptr<bucketdb::ReadGuard<Entry>> acquire_read_guard() const; + /** * Returns true iff bucket has no superbuckets or sub-buckets in the * database. Usage assumption is that any operation that can cause the diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp index bbe477be9ef..c4fb9b8228c 100644 --- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp @@ -250,8 +250,7 @@ GetOperation::assignTargetNodeGroups(const BucketDatabase::ReadGuard& read_guard document::BucketIdFactory bucketIdFactory; document::BucketId bid = bucketIdFactory.getBucketId(_msg->getDocumentId()); - std::vector<BucketDatabase::Entry> entries; - read_guard.find_parents_and_self(bid, entries); + auto entries = read_guard.find_parents_and_self(bid); for (uint32_t j = 0; j < entries.size(); ++j) { const BucketDatabase::Entry& e = entries[j]; |