Merge pull request #3390 from vespa-engine/geirst/fix-bitvector-search-cache-for-imported-attributes

Geirst/fix bitvector search cache for imported attributes

11 files changed, 110 insertions, 12 deletions

diff --git a/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp b/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
index 1f738a209df..0072f83bc54 100644
--- a/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
+++ b/searchlib/src/tests/attribute/bitvector_search_cache/bitvector_search_cache_test.cpp
@@ -3,6 +3,7 @@
 #include <vespa/vespalib/testkit/test_kit.h>
 #include <vespa/searchlib/attribute/bitvector_search_cache.h>
 #include <vespa/searchlib/common/bitvector.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 
 using namespace search;
 using namespace search::attribute;
@@ -13,7 +14,7 @@ using Entry = BitVectorSearchCache::Entry;
 Entry::SP
 makeEntry()
 {
-    return std::make_shared<Entry>(BitVector::create(5), 10);
+    return std::make_shared<Entry>(IDocumentMetaStoreContext::IReadGuard::UP(), BitVector::create(5), 10);
 }
 
 struct Fixture {
diff --git a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
index f6ab6bb50bf..4ab0ed878cf 100644
--- a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
+++ b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
@@ -370,7 +370,7 @@ makeSearchCacheEntry(const std::vector<uint32_t> docIds, uint32_t docIdLimit)
     for (uint32_t docId : docIds) {
         bitVector->setBit(docId);
     }
-    return std::make_shared<BitVectorSearchCache::Entry>(bitVector, docIdLimit);
+    return std::make_shared<BitVectorSearchCache::Entry>(IDocumentMetaStoreContext::IReadGuard::UP(), bitVector, docIdLimit);
 }
 
 TEST_F("Bit vector from search cache is used if found", SearchCacheFixture)
@@ -382,6 +382,7 @@ TEST_F("Bit vector from search cache is used if found", SearchCacheFixture)
     TermFieldMatchData match;
     auto iter = f.create_strict_iterator(*ctx, match);
     TEST_DO(f.assertSearch({2, 6}, *iter)); // Note: would be {3, 5} if cache was not used
+    EXPECT_EQUAL(0u, f.document_meta_store->get_read_guard_cnt);
 }
 
 void
@@ -405,6 +406,7 @@ TEST_F("Entry is inserted into search cache if bit vector posting list is used",
     auto cacheEntry = f.imported_attr->getSearchCache()->find("5678");
     EXPECT_EQUAL(cacheEntry->docIdLimit, f.imported_attr->getNumDocs());
     TEST_DO(assertBitVector({3, 5}, *cacheEntry->bitVector));
+    EXPECT_EQUAL(1u, f.document_meta_store->get_read_guard_cnt);
 }
 
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h b/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
index 73fb95e1752..a889120f8df 100644
--- a/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
+++ b/searchlib/src/vespa/searchlib/attribute/bitvector_search_cache.h
@@ -2,6 +2,7 @@
 
 #pragma once
 
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/vespalib/stllike/hash_map.h>
 #include <vespa/vespalib/stllike/string.h>
 #include <memory>
@@ -21,13 +22,17 @@ namespace attribute {
 class BitVectorSearchCache {
 public:
     using BitVectorSP = std::shared_ptr<BitVector>;
+    using ReadGuardUP = IDocumentMetaStoreContext::IReadGuard::UP;
 
     struct Entry {
         using SP = std::shared_ptr<Entry>;
+        // We need to keep a document meta store read guard to ensure that no lids that are cached
+        // in the bit vector are re-used until the guard is released.
+        ReadGuardUP dmsReadGuard;
         BitVectorSP bitVector;
         uint32_t docIdLimit;
-        Entry(BitVectorSP bitVector_, uint32_t docIdLimit_)
-            : bitVector(std::move(bitVector_)), docIdLimit(docIdLimit_) {}
+        Entry(ReadGuardUP dmsReadGuard_, BitVectorSP bitVector_, uint32_t docIdLimit_)
+            : dmsReadGuard(std::move(dmsReadGuard_)), bitVector(std::move(bitVector_)), docIdLimit(docIdLimit_) {}
     };
 
 private:
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
index 9efac9ce541..a085e21d6ae 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
@@ -14,10 +14,12 @@ ImportedAttributeVector::ImportedAttributeVector(
             vespalib::stringref name,
             std::shared_ptr<ReferenceAttribute> reference_attribute,
             std::shared_ptr<AttributeVector> target_attribute,
+            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
             bool use_search_cache)
     : _name(name),
       _reference_attribute(std::move(reference_attribute)),
       _target_attribute(std::move(target_attribute)),
+      _document_meta_store(std::move(document_meta_store)),
       _search_cache(use_search_cache ? std::make_shared<BitVectorSearchCache>() :
                     std::shared_ptr<BitVectorSearchCache>())
 {
@@ -26,10 +28,12 @@ ImportedAttributeVector::ImportedAttributeVector(
 ImportedAttributeVector::ImportedAttributeVector(vespalib::stringref name,
                                                  std::shared_ptr<ReferenceAttribute> reference_attribute,
                                                  std::shared_ptr<AttributeVector> target_attribute,
+                                                 std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                                                  std::shared_ptr<BitVectorSearchCache> search_cache)
     : _name(name),
       _reference_attribute(std::move(reference_attribute)),
       _target_attribute(std::move(target_attribute)),
+      _document_meta_store(std::move(document_meta_store)),
       _search_cache(std::move(search_cache))
 {
 }
@@ -41,7 +45,7 @@ std::unique_ptr<ImportedAttributeVector>
 ImportedAttributeVector::makeReadGuard(bool stableEnumGuard) const
 {
     return std::make_unique<ImportedAttributeVectorReadGuard>
-        (getName(), getReferenceAttribute(), getTargetAttribute(), getSearchCache(), stableEnumGuard);
+        (getName(), getReferenceAttribute(), getTargetAttribute(), getDocumentMetaStore(), getSearchCache(), stableEnumGuard);
 }
 
 const vespalib::string& search::attribute::ImportedAttributeVector::getName() const {
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
index c94eda3d4db..7d272c42d9e 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
@@ -11,6 +11,7 @@ namespace search {
 
 class AttributeGuard;
 class AttributeEnumGuard;
+class IDocumentMetaStoreContext;
 
 namespace attribute {
 
@@ -32,10 +33,12 @@ public:
     ImportedAttributeVector(vespalib::stringref name,
                             std::shared_ptr<ReferenceAttribute> reference_attribute,
                             std::shared_ptr<AttributeVector> target_attribute,
+                            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                             bool use_search_cache);
     ImportedAttributeVector(vespalib::stringref name,
                             std::shared_ptr<ReferenceAttribute> reference_attribute,
                             std::shared_ptr<AttributeVector> target_attribute,
+                            std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                             std::shared_ptr<BitVectorSearchCache> search_cache);
     ~ImportedAttributeVector();
 
@@ -72,6 +75,9 @@ public:
     const std::shared_ptr<AttributeVector>& getTargetAttribute() const noexcept {
         return _target_attribute;
     }
+    const std::shared_ptr<IDocumentMetaStoreContext> &getDocumentMetaStore() const {
+        return _document_meta_store;
+    }
     const std::shared_ptr<BitVectorSearchCache> &getSearchCache() const {
         return _search_cache;
     }
@@ -88,10 +94,11 @@ protected:
                                       const common::BlobConverter * bc) const override;
 
 
-    vespalib::string                      _name;
-    std::shared_ptr<ReferenceAttribute>   _reference_attribute;
-    std::shared_ptr<AttributeVector>      _target_attribute;
-    std::shared_ptr<BitVectorSearchCache> _search_cache;
+    vespalib::string                           _name;
+    std::shared_ptr<ReferenceAttribute>        _reference_attribute;
+    std::shared_ptr<AttributeVector>           _target_attribute;
+    std::shared_ptr<IDocumentMetaStoreContext> _document_meta_store;
+    std::shared_ptr<BitVectorSearchCache>      _search_cache;
 };
 
 } // attribute
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
index 4ce0f8d340c..c954c34aee8 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
@@ -11,9 +11,11 @@ ImportedAttributeVectorReadGuard::ImportedAttributeVectorReadGuard(
         vespalib::stringref name,
         std::shared_ptr<ReferenceAttribute> reference_attribute,
         std::shared_ptr<AttributeVector> target_attribute,
+        std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
         std::shared_ptr<BitVectorSearchCache> search_cache,
         bool stableEnumGuard)
-    : ImportedAttributeVector(name, std::move(reference_attribute), std::move(target_attribute), std::move(search_cache)),
+    : ImportedAttributeVector(name, std::move(reference_attribute), std::move(target_attribute),
+                              std::move(document_meta_store), std::move(search_cache)),
       _referencedLids(),
       _reference_attribute_guard(_reference_attribute),
       _target_attribute_guard(stableEnumGuard ? std::shared_ptr<AttributeVector>() : _target_attribute),
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
index 27d83614a39..2ca8680f4b8 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
@@ -34,6 +34,7 @@ public:
     ImportedAttributeVectorReadGuard(vespalib::stringref name,
                                      std::shared_ptr<ReferenceAttribute> reference_attribute,
                                      std::shared_ptr<AttributeVector> target_attribute,
+                                     std::shared_ptr<IDocumentMetaStoreContext> document_meta_store,
                                      std::shared_ptr<BitVectorSearchCache> search_cache,
                                      bool stableEnumGuard);
     ~ImportedAttributeVectorReadGuard();
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp b/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
index d2dc52ec2c7..a21ecacccdb 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_search_context.cpp
@@ -29,6 +29,8 @@ ImportedSearchContext::ImportedSearchContext(
       _useSearchCache(_imported_attribute.getSearchCache().get() != nullptr),
       _searchCacheLookup((_useSearchCache ? _imported_attribute.getSearchCache()->find(_queryTerm) :
                           std::shared_ptr<BitVectorSearchCache::Entry>())),
+      _dmsReadGuard((_useSearchCache && !_searchCacheLookup) ? imported_attribute.getDocumentMetaStore()->getReadGuard() :
+                    std::unique_ptr<IDocumentMetaStoreContext::IReadGuard>()),
       _reference_attribute(*_imported_attribute.getReferenceAttribute()),
       _target_attribute(*_imported_attribute.getTargetAttribute()),
       _target_search_context(_target_attribute.getSearch(std::move(term), params)),
@@ -36,6 +38,7 @@ ImportedSearchContext::ImportedSearchContext(
       _merger(_reference_attribute.getCommittedDocIdLimit()),
       _fetchPostingsDone(false)
 {
+
 }
 
 ImportedSearchContext::~ImportedSearchContext() {
@@ -215,7 +218,8 @@ void
 ImportedSearchContext::considerAddSearchCacheEntry()
 {
     if (_useSearchCache && _merger.hasBitVector()) {
-        auto cacheEntry = std::make_shared<BitVectorSearchCache::Entry>(_merger.getBitVectorSP(), _merger.getDocIdLimit());
+        assert(_dmsReadGuard);
+        auto cacheEntry = std::make_shared<BitVectorSearchCache::Entry>(std::move(_dmsReadGuard), _merger.getBitVectorSP(), _merger.getDocIdLimit());
         _imported_attribute.getSearchCache()->insert(_queryTerm, std::move(cacheEntry));
     }
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
index 0f6f8125cf8..0ddca8c9fe5 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
@@ -6,6 +6,7 @@
 #include "bitvector_search_cache.h"
 #include <vespa/searchcommon/attribute/i_search_context.h>
 #include <vespa/searchlib/attribute/posting_list_merger.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/vespalib/util/arrayref.h>
 
 namespace search::fef { class TermFieldMatchData; }
@@ -30,6 +31,7 @@ class ImportedSearchContext : public ISearchContext {
     vespalib::string                                _queryTerm;
     bool                                            _useSearchCache;
     BitVectorSearchCache::Entry::SP                 _searchCacheLookup;
+    IDocumentMetaStoreContext::IReadGuard::UP       _dmsReadGuard;
     const ReferenceAttribute&                       _reference_attribute;
     const AttributeVector&                          _target_attribute;
     std::unique_ptr<AttributeVector::SearchContext> _target_search_context;
diff --git a/searchlib/src/vespa/searchlib/common/i_document_meta_store_context.h b/searchlib/src/vespa/searchlib/common/i_document_meta_store_context.h
new file mode 100644
index 00000000000..7e2e3def0d2
--- /dev/null
+++ b/searchlib/src/vespa/searchlib/common/i_document_meta_store_context.h
@@ -0,0 +1,42 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include <memory>
+
+namespace search {
+
+class IDocumentMetaStore;
+
+/**
+ * API for providing read interface to the document meta store.
+ */
+struct IDocumentMetaStoreContext {
+
+    /**
+     * Guard for access to the read interface.
+     * This guard should be alive as long as read interface is used.
+     */
+    struct IReadGuard {
+
+        typedef std::unique_ptr<IReadGuard> UP;
+
+        virtual ~IReadGuard() {}
+
+        /**
+         * Access to read interface.
+         */
+        virtual const search::IDocumentMetaStore &get() const = 0;
+    };
+
+    virtual ~IDocumentMetaStoreContext() {}
+
+    /**
+     * Access to read interface.
+     * Should be used by all reader threads.
+     */
+    virtual IReadGuard::UP getReadGuard() const = 0;
+
+};
+
+}
diff --git a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
index 3620a1df725..f9e3de79144 100644
--- a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
+++ b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
@@ -12,6 +12,7 @@
 #include <vespa/searchlib/attribute/integerbase.h>
 #include <vespa/searchlib/attribute/not_implemented_attribute.h>
 #include <vespa/searchlib/attribute/stringbase.h>
+#include <vespa/searchlib/common/i_document_meta_store_context.h>
 #include <vespa/searchlib/query/queryterm.h>
 #include <vespa/searchcommon/attribute/attributecontent.h>
 #include <vespa/vespalib/testkit/testapp.h>
@@ -23,6 +24,27 @@
 #include <vector>
 
 namespace search {
+
+struct MockDocumentMetaStoreContext : public IDocumentMetaStoreContext {
+
+    struct MockReadGuard : public IDocumentMetaStoreContext::IReadGuard {
+        virtual const search::IDocumentMetaStore &get() const override {
+            search::IDocumentMetaStore *nullStore = nullptr;
+            return static_cast<search::IDocumentMetaStore &>(*nullStore);
+        }
+    };
+
+    mutable size_t get_read_guard_cnt;
+
+    using SP = std::shared_ptr<MockDocumentMetaStoreContext>;
+    MockDocumentMetaStoreContext() : get_read_guard_cnt(0) {}
+
+    virtual IReadGuard::UP getReadGuard() const override {
+        ++get_read_guard_cnt;
+        return std::make_unique<MockReadGuard>();
+    }
+};
+
 namespace attribute {
 
 using document::DocumentId;
@@ -39,6 +61,10 @@ std::shared_ptr<ReferenceAttribute> create_reference_attribute(vespalib::stringr
     return std::make_shared<ReferenceAttribute>(name, Config(BasicType::REFERENCE));
 }
 
+MockDocumentMetaStoreContext::SP create_document_meta_store() {
+    return std::make_shared<MockDocumentMetaStoreContext>();
+}
+
 enum class FastSearchConfig {
     ExplicitlyEnabled,
     Default
@@ -105,6 +131,7 @@ struct ImportedAttributeFixture {
     bool use_search_cache;
     std::shared_ptr<AttributeVector> target_attr;
     std::shared_ptr<ReferenceAttribute> reference_attr;
+    MockDocumentMetaStoreContext::SP document_meta_store;
     std::shared_ptr<ImportedAttributeVector> imported_attr;
     std::shared_ptr<MockGidToLidMapperFactory> mapper_factory;
 
@@ -130,7 +157,7 @@ struct ImportedAttributeFixture {
 
     std::shared_ptr<ImportedAttributeVector>
     create_attribute_vector_from_members(vespalib::stringref name = default_imported_attr_name()) {
-        return std::make_shared<ImportedAttributeVector>(name, reference_attr, target_attr, use_search_cache);
+        return std::make_shared<ImportedAttributeVector>(name, reference_attr, target_attr, document_meta_store, use_search_cache);
     }
 
     template<typename AttrVecType>
@@ -225,6 +252,7 @@ ImportedAttributeFixture::ImportedAttributeFixture(bool use_search_cache_)
         : use_search_cache(use_search_cache_),
           target_attr(create_single_attribute<IntegerAttribute>(BasicType::INT32)),
           reference_attr(create_reference_attribute()),
+          document_meta_store(create_document_meta_store()),
           imported_attr(create_attribute_vector_from_members()),
           mapper_factory(std::make_shared<MockGidToLidMapperFactory>()) {
     reference_attr->setGidToLidMapperFactory(mapper_factory);