diff options
author | Geir Storli <geirstorli@yahoo.no> | 2017-09-12 15:53:35 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-09-12 15:53:35 +0200 |
commit | 664dc8e147928cd05a16954cbfdd6efb34dd144a (patch) | |
tree | a5b985aab6b38f3b955c5b62469a9bcbd8a4c700 /searchlib | |
parent | 5ca40d533eabdb8fad2d0951285182b998aaf9df (diff) | |
parent | 093d8381783f0acac2b53be4703fd61189d0b470 (diff) |
Merge pull request #3390 from vespa-engine/geirst/fix-bitvector-search-cache-for-imported-attributes
Geirst/fix bitvector search cache for imported attributes
Diffstat (limited to 'searchlib')
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); |