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);