diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2022-08-19 13:36:55 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2022-08-19 13:36:55 +0000 |
commit | a3b1279be88fa1414922f032861baee36eb9c19a (patch) | |
tree | 6a02aa3df417993bcf2592a783964f69e90d1b00 | |
parent | f0790b529463aba73d3e37f738d96529f79172d1 (diff) |
Avoid copying a vector of shared_ptrs when lifetime is guaranteed by the lock held.
6 files changed, 107 insertions, 40 deletions
diff --git a/searchcore/src/vespa/searchcore/proton/common/handlermap.hpp b/searchcore/src/vespa/searchcore/proton/common/handlermap.hpp index 4e78bf04df6..697028e42cc 100644 --- a/searchcore/src/vespa/searchcore/proton/common/handlermap.hpp +++ b/searchcore/src/vespa/searchcore/proton/common/handlermap.hpp @@ -31,7 +31,7 @@ public: * using event barriers to resolve visibility constraints in the * future without changing the external API. **/ - class Snapshot : public vespalib::Sequence<T*> + class Snapshot final : public vespalib::Sequence<T*> { private: std::vector<HandlerSP> _handlers; @@ -41,7 +41,7 @@ public: Snapshot() : _handlers(), _offset(0) { } Snapshot(const StdMap &map) : _handlers(), _offset(0) { _handlers.reserve(map.size()); - for (auto itr : map) { + for (const auto & itr : map) { _handlers.push_back(itr.second); } } @@ -51,11 +51,39 @@ public: Snapshot(const Snapshot &) = delete; Snapshot & operator = (const Snapshot &) = delete; + size_t size() const { return _handlers.size(); } + bool valid() const override { return (_offset < _handlers.size()); } T *get() const override { return _handlers[_offset].get(); } void next() override { ++_offset; } }; + class UnsafeSnapshot final : public vespalib::Sequence<T*> + { + private: + std::vector<T *> _handlers; + size_t _offset; + + public: + UnsafeSnapshot() : _handlers(), _offset(0) { } + UnsafeSnapshot(const StdMap &map) : _handlers(), _offset(0) { + _handlers.reserve(map.size()); + for (const auto & itr : map) { + _handlers.push_back(itr.second.get()); + } + } + UnsafeSnapshot(UnsafeSnapshot &&) noexcept = default; + UnsafeSnapshot & operator = (UnsafeSnapshot &&) noexcept = default; + UnsafeSnapshot(const UnsafeSnapshot &) = delete; + UnsafeSnapshot & operator = (const UnsafeSnapshot &) = delete; + + size_t size() const { return _handlers.size(); } + + bool valid() const override { return (_offset < _handlers.size()); } + T *get() const override { return _handlers[_offset]; } + void next() override { ++_offset; } + }; + /** * Convenience typedefs. */ @@ -170,12 +198,19 @@ public: **/ Snapshot snapshot() const { return Snapshot(_handlers); } + /** + * Create a snapshot of the handlers currently contained in this + * map and return it as a sequence. Note that any lifetime guarantees + * must be be given at a higher level + * + * @return handler sequence + **/ + UnsafeSnapshot unsafeSnapshot() const { return UnsafeSnapshot(_handlers); } + // we want to use snapshots rather than direct iteration to reduce locking; // the below functions should be deprecated when possible. - iterator begin() { return _handlers.begin(); } const_iterator begin() const { return _handlers.begin(); } - iterator end() { return _handlers.end(); } const_iterator end() const { return _handlers.end(); } size_t size() const { return _handlers.size(); } }; diff --git a/searchcore/src/vespa/searchcore/proton/flushengine/flushengine.cpp b/searchcore/src/vespa/searchcore/proton/flushengine/flushengine.cpp index 011d97d4609..fd3deb2abd7 100644 --- a/searchcore/src/vespa/searchcore/proton/flushengine/flushengine.cpp +++ b/searchcore/src/vespa/searchcore/proton/flushengine/flushengine.cpp @@ -367,7 +367,7 @@ FlushEngine::initFlush(const FlushContext &ctx) void FlushEngine::flushDone(const FlushContext &ctx, uint32_t taskId) { - vespalib::duration duration = vespalib::duration::zero(); + vespalib::duration duration; { std::lock_guard<std::mutex> guard(_lock); duration = _flushing[taskId].elapsed(); @@ -422,7 +422,7 @@ FlushEngine::getCurrentlyFlushingSet() const uint32_t FlushEngine::initFlush(const IFlushHandler::SP &handler, const IFlushTarget::SP &target) { - uint32_t taskId(0); + uint32_t taskId; { std::lock_guard<std::mutex> guard(_lock); taskId = _taskId++; diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.cpp index cc619c3a09a..1bcb96702a4 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.cpp +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.cpp @@ -6,6 +6,7 @@ namespace proton { using HandlerSnapshot = PersistenceHandlerMap::HandlerSnapshot; +using UnsafeHandlerSnapshot = PersistenceHandlerMap::UnsafeHandlerSnapshot; PersistenceHandlerMap::PersistenceHandlerMap() : _map() @@ -51,8 +52,7 @@ PersistenceHandlerMap::getHandlerSnapshot() const handlers.push_back(handlerItr.second); } } - size_t handlersSize = handlers.size(); - return HandlerSnapshot(DocTypeToHandlerMap::Snapshot(std::move(handlers)), handlersSize); + return HandlerSnapshot(DocTypeToHandlerMap::Snapshot(std::move(handlers))); } HandlerSnapshot @@ -60,9 +60,22 @@ PersistenceHandlerMap::getHandlerSnapshot(document::BucketSpace bucketSpace) con { auto itr = _map.find(bucketSpace); if (itr != _map.end()) { - return HandlerSnapshot(itr->second.snapshot(), itr->second.size()); + return HandlerSnapshot(itr->second.snapshot()); } return HandlerSnapshot(); } +UnsafeHandlerSnapshot +PersistenceHandlerMap::getUnsafeHandlerSnapshot(document::BucketSpace bucketSpace) const +{ + auto itr = _map.find(bucketSpace); + if (itr != _map.end()) { + return UnsafeHandlerSnapshot(itr->second.unsafeSnapshot()); + } + return UnsafeHandlerSnapshot(); +} + +PersistenceHandlerMap::HandlerSnapshot::~HandlerSnapshot() = default; +PersistenceHandlerMap::UnsafeHandlerSnapshot::~UnsafeHandlerSnapshot() = default; + } diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.h b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.h index 749f044ff0e..807b0f0b5d7 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.h +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistence_handler_map.h @@ -21,30 +21,42 @@ class IPersistenceHandler; class PersistenceHandlerMap { public: using DocTypeToHandlerMap = HandlerMap<IPersistenceHandler>; - using PersistenceHandlerSequence = DocTypeToHandlerMap::Snapshot; using PersistenceHandlerSP = std::shared_ptr<IPersistenceHandler>; class HandlerSnapshot { private: - PersistenceHandlerSequence _handlers; - size_t _size; + DocTypeToHandlerMap::Snapshot _handlers; public: - HandlerSnapshot() : _handlers(), _size(0) {} - HandlerSnapshot(DocTypeToHandlerMap::Snapshot handlers_, size_t size_) - : _handlers(std::move(handlers_)), - _size(size_) + HandlerSnapshot() : _handlers() {} + HandlerSnapshot(DocTypeToHandlerMap::Snapshot handlers_) + : _handlers(std::move(handlers_)) {} HandlerSnapshot(const HandlerSnapshot &) = delete; HandlerSnapshot & operator = (const HandlerSnapshot &) = delete; + ~HandlerSnapshot(); - size_t size() const { return _size; } - PersistenceHandlerSequence &handlers() { return _handlers; } - static PersistenceHandlerSequence release(HandlerSnapshot &&rhs) { return std::move(rhs._handlers); } + size_t size() const { return _handlers.size(); } + vespalib::Sequence<IPersistenceHandler*> &handlers() { return _handlers; } + static DocTypeToHandlerMap::Snapshot release(HandlerSnapshot &&rhs) { return std::move(rhs._handlers); } }; -private: + class UnsafeHandlerSnapshot { + private: + DocTypeToHandlerMap::UnsafeSnapshot _handlers; + public: + UnsafeHandlerSnapshot() : _handlers() {} + UnsafeHandlerSnapshot(DocTypeToHandlerMap::UnsafeSnapshot handlers_) + : _handlers(std::move(handlers_)) + {} + UnsafeHandlerSnapshot(const UnsafeHandlerSnapshot &) = delete; + UnsafeHandlerSnapshot & operator = (const UnsafeHandlerSnapshot &) = delete; + ~UnsafeHandlerSnapshot(); + size_t size() const { return _handlers.size(); } + vespalib::Sequence<IPersistenceHandler*> &handlers() { return _handlers; } + }; +private: struct BucketSpaceHash { std::size_t operator() (const document::BucketSpace &bucketSpace) const { return bucketSpace.getId(); } }; @@ -59,6 +71,7 @@ public: IPersistenceHandler * getHandler(document::BucketSpace bucketSpace, const DocTypeName &docType) const; HandlerSnapshot getHandlerSnapshot() const; HandlerSnapshot getHandlerSnapshot(document::BucketSpace bucketSpace) const; + UnsafeHandlerSnapshot getUnsafeHandlerSnapshot(document::BucketSpace bucketSpace) const; }; } diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp index c6d9bbd82b9..df3dcdcf66a 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp @@ -199,11 +199,16 @@ PersistenceEngine::getHandlerSnapshot(const WriteGuard &) const } PersistenceEngine::HandlerSnapshot -PersistenceEngine::getHandlerSnapshot(const ReadGuard &, document::BucketSpace bucketSpace) const +PersistenceEngine::getSafeHandlerSnapshot(const ReadGuard &, document::BucketSpace bucketSpace) const { return _handlers.getHandlerSnapshot(bucketSpace); } +PersistenceEngine::UnsafeHandlerSnapshot +PersistenceEngine::getHandlerSnapshot(const ReadGuard &, document::BucketSpace bucketSpace) const { + return _handlers.getUnsafeHandlerSnapshot(bucketSpace); +} + PersistenceEngine::HandlerSnapshot PersistenceEngine::getHandlerSnapshot(const WriteGuard &, document::BucketSpace bucketSpace) const { @@ -278,7 +283,7 @@ PersistenceEngine::listBuckets(BucketSpace bucketSpace) const // Runs in SPI thread. // No handover to write threads in persistence handlers. ReadGuard rguard(_rwMutex); - HandlerSnapshot snap = getHandlerSnapshot(rguard, bucketSpace); + auto snap = getHandlerSnapshot(rguard, bucketSpace); BucketIdListResultHandler resultHandler; for (; snap.handlers().valid(); snap.handlers().next()) { IPersistenceHandler *handler = snap.handlers().get(); @@ -293,7 +298,7 @@ PersistenceEngine::setClusterState(BucketSpace bucketSpace, const ClusterState & { ReadGuard rguard(_rwMutex); saveClusterState(bucketSpace, calc); - HandlerSnapshot snap = getHandlerSnapshot(rguard, bucketSpace); + auto snap = getHandlerSnapshot(rguard, bucketSpace); auto catchResult = std::make_unique<storage::spi::CatchResult>(); auto futureResult = catchResult->future_result(); GenericResultHandler resultHandler(snap.size(), std::move(catchResult)); @@ -311,7 +316,7 @@ void PersistenceEngine::setActiveStateAsync(const Bucket & bucket, BucketInfo::ActiveState newState, OperationComplete::UP onComplete) { ReadGuard rguard(_rwMutex); - HandlerSnapshot snap = getHandlerSnapshot(rguard, bucket.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, bucket.getBucketSpace()); auto resultHandler = std::make_shared<GenericResultHandler>(snap.size(), std::move(onComplete)); while (snap.handlers().valid()) { IPersistenceHandler *handler = snap.handlers().get(); @@ -331,7 +336,7 @@ PersistenceEngine::getBucketInfo(const Bucket& b) const // Runs in SPI thread. // No handover to write threads in persistence handlers. ReadGuard rguard(_rwMutex); - HandlerSnapshot snap = getHandlerSnapshot(rguard, b.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, b.getBucketSpace()); BucketInfoResultHandler resultHandler; for (; snap.handlers().valid(); snap.handlers().next()) { IPersistenceHandler *handler = snap.handlers().get(); @@ -481,10 +486,10 @@ PersistenceEngine::GetResult PersistenceEngine::get(const Bucket& b, const document::FieldSet& fields, const DocumentId& did, Context& context) const { ReadGuard rguard(_rwMutex); - HandlerSnapshot snapshot = getHandlerSnapshot(rguard, b.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, b.getBucketSpace()); - for (PersistenceHandlerSequence & handlers = snapshot.handlers(); handlers.valid(); handlers.next()) { - IPersistenceHandler::RetrieversSP retrievers = handlers.get()->getDocumentRetrievers(context.getReadConsistency()); + for (;snap.handlers().valid(); snap.handlers().next()) { + IPersistenceHandler::RetrieversSP retrievers = snap.handlers().get()->getDocumentRetrievers(context.getReadConsistency()); for (size_t i = 0; i < retrievers->size(); ++i) { IDocumentRetriever &retriever = *(*retrievers)[i]; search::DocumentMetaData meta = retriever.getDocumentMetaData(did); @@ -513,17 +518,17 @@ PersistenceEngine::createIterator(const Bucket &bucket, FieldSetSP fields, const IncludedVersions versions, Context &context) { ReadGuard rguard(_rwMutex); - HandlerSnapshot snapshot = getHandlerSnapshot(rguard, bucket.getBucketSpace()); + auto snap = getSafeHandlerSnapshot(rguard, bucket.getBucketSpace()); auto entry = std::make_unique<IteratorEntry>(context.getReadConsistency(), bucket, std::move(fields), selection, versions, _defaultSerializedSize, _ignoreMaxBytes); - for (PersistenceHandlerSequence & handlers = snapshot.handlers(); handlers.valid(); handlers.next()) { - IPersistenceHandler::RetrieversSP retrievers = handlers.get()->getDocumentRetrievers(context.getReadConsistency()); + for (; snap.handlers().valid(); snap.handlers().next()) { + IPersistenceHandler::RetrieversSP retrievers = snap.handlers().get()->getDocumentRetrievers(context.getReadConsistency()); for (const auto & retriever : *retrievers) { entry->it.add(retriever); } } - entry->handler_sequence = HandlerSnapshot::release(std::move(snapshot)); + entry->handler_sequence = HandlerSnapshot::release(std::move(snap)); std::lock_guard<std::mutex> guard(_iterators_lock); static std::atomic<IteratorId::Type> id_counter(0); @@ -590,7 +595,7 @@ PersistenceEngine::createBucketAsync(const Bucket &b, OperationComplete::UP onCo { ReadGuard rguard(_rwMutex); LOG(spam, "createBucket(%s)", b.toString().c_str()); - HandlerSnapshot snap = getHandlerSnapshot(rguard, b.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, b.getBucketSpace()); auto transportContext = std::make_shared<AsyncTransportContext>(snap.size(), std::move(onComplete)); while (snap.handlers().valid()) { @@ -610,7 +615,7 @@ PersistenceEngine::deleteBucketAsync(const Bucket& b, OperationComplete::UP onCo { ReadGuard rguard(_rwMutex); LOG(spam, "deleteBucket(%s)", b.toString().c_str()); - HandlerSnapshot snap = getHandlerSnapshot(rguard, b.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, b.getBucketSpace()); auto transportContext = std::make_shared<AsyncTransportContext>(snap.size(), std::move(onComplete)); while (snap.handlers().valid()) { @@ -635,7 +640,7 @@ PersistenceEngine::getModifiedBuckets(BucketSpace bucketSpace) const std::lock_guard<std::mutex> guard(_lock); extraModifiedBuckets.swap(_extraModifiedBuckets[bucketSpace]); } - HandlerSnapshot snap = getHandlerSnapshot(rguard, bucketSpace); + auto snap = getHandlerSnapshot(rguard, bucketSpace); auto catchResult = std::make_unique<storage::spi::CatchResult>(); auto futureResult = catchResult->future_result(); SynchronizedBucketIdListResultHandler resultHandler(snap.size() + extraModifiedBuckets.size(), std::move(catchResult)); @@ -657,7 +662,7 @@ PersistenceEngine::split(const Bucket& source, const Bucket& target1, const Buck LOG(spam, "split(%s, %s, %s)", source.toString().c_str(), target1.toString().c_str(), target2.toString().c_str()); assert(source.getBucketSpace() == target1.getBucketSpace()); assert(source.getBucketSpace() == target2.getBucketSpace()); - HandlerSnapshot snap = getHandlerSnapshot(rguard, source.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, source.getBucketSpace()); TransportLatch latch(snap.size()); for (; snap.handlers().valid(); snap.handlers().next()) { IPersistenceHandler *handler = snap.handlers().get(); @@ -675,7 +680,7 @@ PersistenceEngine::join(const Bucket& source1, const Bucket& source2, const Buck LOG(spam, "join(%s, %s, %s)", source1.toString().c_str(), source2.toString().c_str(), target.toString().c_str()); assert(source1.getBucketSpace() == target.getBucketSpace()); assert(source2.getBucketSpace() == target.getBucketSpace()); - HandlerSnapshot snap = getHandlerSnapshot(rguard, target.getBucketSpace()); + auto snap = getHandlerSnapshot(rguard, target.getBucketSpace()); TransportLatch latch(snap.size()); for (; snap.handlers().valid(); snap.handlers().next()) { IPersistenceHandler *handler = snap.handlers().get(); diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h index c16cc6e6a83..2581d35064e 100644 --- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h +++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h @@ -19,8 +19,9 @@ class IDiskMemUsageNotifier; class PersistenceEngine : public storage::spi::AbstractPersistenceProvider, public storage::spi::BucketExecutor { private: - using PersistenceHandlerSequence = PersistenceHandlerMap::PersistenceHandlerSequence; + using PersistenceHandlerSequence = PersistenceHandlerMap::DocTypeToHandlerMap::Snapshot; using HandlerSnapshot = PersistenceHandlerMap::HandlerSnapshot; + using UnsafeHandlerSnapshot = PersistenceHandlerMap::UnsafeHandlerSnapshot; using DocumentUpdate = document::DocumentUpdate; using Bucket = storage::spi::Bucket; using BucketIdListResult = storage::spi::BucketIdListResult; @@ -38,7 +39,6 @@ private: using Result = storage::spi::Result; using Selection = storage::spi::Selection; using Timestamp = storage::spi::Timestamp; - using TimestampList = storage::spi::TimestampList; using UpdateResult = storage::spi::UpdateResult; using OperationComplete = storage::spi::OperationComplete; using BucketExecutor = storage::spi::BucketExecutor; @@ -82,7 +82,8 @@ private: IPersistenceHandler * getHandler(const ReadGuard & guard, document::BucketSpace bucketSpace, const DocTypeName &docType) const; HandlerSnapshot getHandlerSnapshot(const WriteGuard & guard) const; - HandlerSnapshot getHandlerSnapshot(const ReadGuard & guard, document::BucketSpace bucketSpace) const; + HandlerSnapshot getSafeHandlerSnapshot(const ReadGuard & guard, document::BucketSpace bucketSpace) const; + UnsafeHandlerSnapshot getHandlerSnapshot(const ReadGuard & guard, document::BucketSpace bucketSpace) const; HandlerSnapshot getHandlerSnapshot(const WriteGuard & guard, document::BucketSpace bucketSpace) const; void saveClusterState(BucketSpace bucketSpace, const ClusterState &calc); |