Merge pull request #3158 from vespa-engine/toregge/add-imported-attribute-vector-read-guard

Add ImportedAttributeVectorReadGuard, used to handle imported attributes

8 files changed, 255 insertions, 57 deletions

diff --git a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
index 306030dbe3e..7a9d877621c 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
@@ -12,23 +12,6 @@ using LockGuard = std::lock_guard<std::mutex>;
 
 namespace proton {
 
-ImportedAttributesContext::GuardedAttribute::GuardedAttribute(ImportedAttributeVector::SP attr,
-                                                              bool stableEnumGuard)
-    : _attr(std::move(attr)),
-      _guard(stableEnumGuard ? _attr->acquireEnumGuard() : _attr->acquireGuard())
-{
-}
-
-ImportedAttributesContext::GuardedAttribute::~GuardedAttribute()
-{
-}
-
-const IAttributeVector *
-ImportedAttributesContext::GuardedAttribute::get() const
-{
-    return _attr.get();
-}
-
 const IAttributeVector *
 ImportedAttributesContext::getOrCacheAttribute(const vespalib::string &name,
                                                AttributeCache &attributes,
@@ -41,8 +24,8 @@ ImportedAttributesContext::getOrCacheAttribute(const vespalib::string &name,
     }
     ImportedAttributeVector::SP result = _repo.get(name);
     if (result.get() != nullptr) {
-        attributes.emplace(name, GuardedAttribute(result, stableEnumGuard));
-        return result.get();
+        auto insRes = attributes.emplace(name, result->makeReadGuard(stableEnumGuard));
+        return insRes.first->second.get();
     } else {
         return nullptr;
     }
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
index c8c9d021a5f..5daed203446 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
+++ b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
@@ -29,20 +29,7 @@ private:
     using IAttributeVector = search::attribute::IAttributeVector;
     using ImportedAttributeVector = search::attribute::ImportedAttributeVector;
 
-    class GuardedAttribute {
-    private:
-        std::shared_ptr<ImportedAttributeVector> _attr;
-        std::unique_ptr<AttributeGuard> _guard;
-
-    public:
-        GuardedAttribute(std::shared_ptr<ImportedAttributeVector> attr, bool stableEnumGuard);
-        ~GuardedAttribute();
-        GuardedAttribute(GuardedAttribute &&rhs) = default;
-        GuardedAttribute &operator=(GuardedAttribute &&rhs) = default;
-        const IAttributeVector *get() const;
-    };
-
-    using AttributeCache = std::unordered_map<vespalib::string, GuardedAttribute, vespalib::hash<vespalib::string>>;
+    using AttributeCache = std::unordered_map<vespalib::string, std::unique_ptr<ImportedAttributeVector>, vespalib::hash<vespalib::string>>;
     using LockGuard = std::lock_guard<std::mutex>;
 
     const ImportedAttributesRepo &_repo;
diff --git a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
index 332892aeaea..747db9e2527 100644
--- a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
+++ b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
@@ -7,7 +7,26 @@
 namespace search {
 namespace attribute {
 
-using Fixture = ImportedAttributeFixture;
+
+template <bool useReadGuard = false>
+struct FixtureBase : public ImportedAttributeFixture
+{
+    FixtureBase()
+        : ImportedAttributeFixture()
+    {
+    }
+
+    std::shared_ptr<ImportedAttributeVector> get_imported_attr() {
+        if (useReadGuard) {
+            return imported_attr->makeReadGuard(false);
+        } else {
+            return imported_attr;
+        }
+    }
+};
+
+using Fixture = FixtureBase<false>;
+using ReadGuardFixture = FixtureBase<true>;
 
 TEST_F("Accessors return expected attributes", Fixture) {
     EXPECT_EQUAL(f.imported_attr->getReferenceAttribute().get(),
@@ -43,12 +62,12 @@ TEST_F("getBasicType() returns target vector basic type", Fixture) {
     EXPECT_EQUAL(BasicType::DOUBLE, f.imported_attr->getBasicType());
 }
 
-TEST_F("acquireGuard() acquires guards on both target and reference attributes", Fixture) {
+TEST_F("makeReadGuard(false) acquires guards on both target and reference attributes", Fixture) {
     add_n_docs_with_undefined_values(*f.reference_attr, 2);
     add_n_docs_with_undefined_values(*f.target_attr, 2);
     // Now at generation 1 in both attributes.
     {
-        auto guard = f.imported_attr->acquireGuard();
+        auto guard = f.imported_attr->makeReadGuard(false);
         add_n_docs_with_undefined_values(*f.reference_attr, 1);
         add_n_docs_with_undefined_values(*f.target_attr, 1);
         
@@ -65,12 +84,12 @@ TEST_F("acquireGuard() acquires guards on both target and reference attributes",
     EXPECT_EQUAL(3u, f.reference_attr->getFirstUsedGeneration());
 }
 
-TEST_F("acquireEnumGuard() acquires enum guard on target and regular guard on reference attribute", Fixture) {
+TEST_F("makeReadGuard(true) acquires enum guard on target and regular guard on reference attribute", Fixture) {
     f.reset_with_new_target_attr(create_single_attribute<StringAttribute>(BasicType::STRING));
     add_n_docs_with_undefined_values(*f.reference_attr, 2);
     add_n_docs_with_undefined_values(*f.target_attr, 2);
     {
-        auto guard = f.imported_attr->acquireEnumGuard();
+        auto guard = f.imported_attr->makeReadGuard(true);
         add_n_docs_with_undefined_values(*f.target_attr, 1);
         add_n_docs_with_undefined_values(*f.reference_attr, 1);
 
@@ -89,25 +108,52 @@ TEST_F("acquireEnumGuard() acquires enum guard on target and regular guard on re
     EXPECT_FALSE(has_active_enum_guards(*f.target_attr));
 }
 
-TEST_F("Single-valued integer attribute values can be retrieved via reference", Fixture) {
+template <typename Fixture>
+void
+checkSingleInt()
+{
+    Fixture f;
     reset_with_single_value_reference_mappings<IntegerAttribute, int32_t>(
             f, BasicType::INT32,
             {{DocId(1), dummy_gid(3), DocId(3), 1234},
              {DocId(3), dummy_gid(7), DocId(7), 5678}});
 
-    EXPECT_EQUAL(1234, f.imported_attr->getInt(DocId(1)));
-    EXPECT_EQUAL(5678, f.imported_attr->getInt(DocId(3)));
+    EXPECT_EQUAL(1234, f.get_imported_attr()->getInt(DocId(1)));
+    EXPECT_EQUAL(5678, f.get_imported_attr()->getInt(DocId(3)));
+}
+
+TEST("Single-valued integer attribute values can be retrieved via reference") {
+    TEST_DO(checkSingleInt<Fixture>());
+    TEST_DO(checkSingleInt<ReadGuardFixture>());
 }
 
-TEST_F("getValueCount() is 1 for mapped single value attribute", Fixture) {
+template <typename Fixture>
+void
+checkSingleMappedValueCount()
+{
+    Fixture f;
     reset_with_single_value_reference_mappings<IntegerAttribute, int32_t>(
             f, BasicType::INT32, {{DocId(1), dummy_gid(3), DocId(3), 1234}});
-    EXPECT_EQUAL(1u, f.imported_attr->getValueCount(DocId(1)));
+    EXPECT_EQUAL(1u, f.get_imported_attr()->getValueCount(DocId(1)));
+}
+
+TEST("getValueCount() is 1 for mapped single value attribute") {
+    TEST_DO(checkSingleMappedValueCount<Fixture>());
+    TEST_DO(checkSingleMappedValueCount<ReadGuardFixture>());
 }
 
-TEST_F("getValueCount() is 0 for non-mapped single value attribute", Fixture) {
+template <typename Fixture>
+void
+checkSingleNonMappedValueCount()
+{
+    Fixture f;
     add_n_docs_with_undefined_values(*f.reference_attr, 3);
-    EXPECT_EQUAL(0u, f.imported_attr->getValueCount(DocId(2)));
+    EXPECT_EQUAL(0u, f.get_imported_attr()->getValueCount(DocId(2)));
+}
+
+TEST("getValueCount() is 0 for non-mapped single value attribute") {
+    TEST_DO(checkSingleNonMappedValueCount<Fixture>());
+    TEST_DO(checkSingleNonMappedValueCount<ReadGuardFixture>());
 }
 
 TEST_F("getMaxValueCount() is 1 for single value attribute vectors", Fixture) {
@@ -148,11 +194,20 @@ TEST_F("Weighted integer attribute values can be retrieved via reference", Fixtu
     assert_multi_value_matches<WeightedInt>(f, DocId(3), doc7_values);
 }
 
-TEST_F("LID with not present GID reference mapping returns default value", Fixture) {
+template <class Fixture>
+void
+checkLidWithNotPresentGid()
+{
+    Fixture f;
     f.target_attr->addReservedDoc();
     add_n_docs_with_undefined_values(*f.reference_attr, 2);
     EXPECT_EQUAL(f.target_attr->getInt(DocId(0)), // Implicit default undefined value
-                 f.imported_attr->getInt(DocId(1)));
+                 f.get_imported_attr()->getInt(DocId(1)));
+}
+
+TEST("LID with not present GID reference mapping returns default value") {
+    TEST_DO(checkLidWithNotPresentGid<Fixture>());
+    TEST_DO(checkLidWithNotPresentGid<ReadGuardFixture>());
 }
 
 TEST_F("Singled-valued floating point attribute values can be retrieved via reference", Fixture) {
@@ -187,30 +242,53 @@ TEST_F("Weighted floating point attribute values can be retrieved via reference"
     assert_multi_value_matches<WeightedFloat>(f, DocId(3), doc7_values);
 }
 
-struct SingleStringAttrFixture : Fixture {
-    SingleStringAttrFixture() : Fixture() {
+template <bool useReadGuard = false>
+struct SingleStringAttrFixtureBase : FixtureBase<useReadGuard> {
+    SingleStringAttrFixtureBase() : FixtureBase<useReadGuard>() {
         setup();
     }
 
     void setup() {
-        reset_with_single_value_reference_mappings<StringAttribute, const char*>(
+        this->template reset_with_single_value_reference_mappings<StringAttribute, const char*>(
                 BasicType::STRING,
                 {{DocId(2), dummy_gid(3), DocId(3), "foo"},
                  {DocId(4), dummy_gid(7), DocId(7), "bar"}});
     }
 };
 
-TEST_F("Single-valued string attribute values can be retrieved via reference", SingleStringAttrFixture) {
+using SingleStringAttrFixture = SingleStringAttrFixtureBase<false>;
+using ReadGuardSingleStringAttrFixture = SingleStringAttrFixtureBase<true>;
+
+template <class Fixture>
+void
+checkSingleString()
+{
+    Fixture f;
     char buf[64];
-    EXPECT_EQUAL(vespalib::string("foo"), f.imported_attr->getString(DocId(2), buf, sizeof(buf)));
-    EXPECT_EQUAL(vespalib::string("bar"), f.imported_attr->getString(DocId(4), buf, sizeof(buf)));
+    EXPECT_EQUAL(vespalib::string("foo"), f.get_imported_attr()->getString(DocId(2), buf, sizeof(buf)));
+    EXPECT_EQUAL(vespalib::string("bar"), f.get_imported_attr()->getString(DocId(4), buf, sizeof(buf)));
 }
 
-TEST_F("getEnum() returns target vector enum via reference", SingleStringAttrFixture) {
+
+TEST("Single-valued string attribute values can be retrieved via reference") {
+    TEST_DO(checkSingleString<SingleStringAttrFixture>());
+    TEST_DO(checkSingleString<ReadGuardSingleStringAttrFixture>());
+}
+
+template <class Fixture>
+void
+checkSingleStringEnum()
+{
+    Fixture f;
     EXPECT_EQUAL(f.target_attr->getEnum(DocId(3)),
-                 f.imported_attr->getEnum(DocId(2)));
+                 f.get_imported_attr()->getEnum(DocId(2)));
     EXPECT_EQUAL(f.target_attr->getEnum(DocId(7)),
-                 f.imported_attr->getEnum(DocId(4)));
+                 f.get_imported_attr()->getEnum(DocId(4)));
+}
+
+TEST("getEnum() returns target vector enum via reference") {
+    TEST_DO(checkSingleStringEnum<SingleStringAttrFixture>());
+    TEST_DO(checkSingleStringEnum<ReadGuardSingleStringAttrFixture>());
 }
 
 TEST_F("findEnum() returns target vector enum via reference", SingleStringAttrFixture) {
diff --git a/searchlib/src/vespa/searchlib/attribute/CMakeLists.txt b/searchlib/src/vespa/searchlib/attribute/CMakeLists.txt
index 6838119ccd9..7f693b9092d 100644
--- a/searchlib/src/vespa/searchlib/attribute/CMakeLists.txt
+++ b/searchlib/src/vespa/searchlib/attribute/CMakeLists.txt
@@ -46,6 +46,7 @@ vespa_add_library(searchlib_attribute OBJECT
     iattributemanager.cpp
     iattributesavetarget.cpp
     imported_attribute_vector.cpp
+    imported_attribute_vector_read_guard.cpp
     imported_search_context.cpp
     integerbase.cpp
     ipostinglistsearchcontext.cpp
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
index ecccba43bb0..2aeb2ee0cfe 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "imported_attribute_vector.h"
+#include "imported_attribute_vector_read_guard.h"
 #include "imported_search_context.h"
 #include "attributeguard.h"
 #include <vespa/searchlib/query/queryterm.h>
@@ -22,6 +23,13 @@ ImportedAttributeVector::ImportedAttributeVector(
 ImportedAttributeVector::~ImportedAttributeVector() {
 }
 
+std::unique_ptr<ImportedAttributeVector>
+ImportedAttributeVector::makeReadGuard(bool stableEnumGuard) const
+{
+    return std::make_unique<ImportedAttributeVectorReadGuard>
+        (getName(), getReferenceAttribute(), getTargetAttribute(), stableEnumGuard);
+}
+
 const vespalib::string& search::attribute::ImportedAttributeVector::getName() const {
     return _name;
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
index 1a20d395bd3..fe9a0d12d6f 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.h
@@ -80,7 +80,11 @@ public:
      */
     std::unique_ptr<AttributeEnumGuard> acquireEnumGuard() const;
 
-private:
+    /*
+     * Create an imported attribute with a snapshot of lid to lid mapping.
+     */
+    std::unique_ptr<ImportedAttributeVector> makeReadGuard(bool stableEnumGuard) const;
+protected:
     long onSerializeForAscendingSort(DocId doc, void * serTo, long available,
                                      const common::BlobConverter * bc) const override;
     long onSerializeForDescendingSort(DocId doc, void * serTo, long available,
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
new file mode 100644
index 00000000000..85f8e980026
--- /dev/null
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
@@ -0,0 +1,84 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "imported_attribute_vector_read_guard.h"
+
+namespace search {
+namespace attribute {
+
+ImportedAttributeVectorReadGuard::ImportedAttributeVectorReadGuard(
+        vespalib::stringref name,
+        std::shared_ptr<ReferenceAttribute> reference_attribute,
+        std::shared_ptr<AttributeVector> target_attribute,
+        bool stableEnumGuard)
+    : ImportedAttributeVector(name, std::move(reference_attribute), std::move(target_attribute)),
+      _referencedLids(),
+      _referencedLidLimit(0u),
+      _reference_attribute_guard(_reference_attribute),
+      _target_attribute_guard(stableEnumGuard ? std::shared_ptr<AttributeVector>() : _target_attribute),
+      _target_attribute_enum_guard(stableEnumGuard ? _target_attribute : std::shared_ptr<AttributeVector>())
+{
+    _referencedLids = _reference_attribute->getReferencedLids();
+    _referencedLidLimit = _target_attribute->getCommittedDocIdLimit();
+}
+
+ImportedAttributeVectorReadGuard::~ImportedAttributeVectorReadGuard() {
+}
+
+uint32_t ImportedAttributeVectorReadGuard::getValueCount(uint32_t doc) const {
+    return _target_attribute->getValueCount(getReferencedLid(doc));
+}
+
+IAttributeVector::largeint_t ImportedAttributeVectorReadGuard::getInt(DocId doc) const {
+    return _target_attribute->getInt(getReferencedLid(doc));
+}
+
+double ImportedAttributeVectorReadGuard::getFloat(DocId doc) const {
+    return _target_attribute->getFloat(getReferencedLid(doc));
+}
+
+const char *ImportedAttributeVectorReadGuard::getString(DocId doc, char *buffer, size_t sz) const {
+    return _target_attribute->getString(getReferencedLid(doc), buffer, sz);
+}
+
+IAttributeVector::EnumHandle ImportedAttributeVectorReadGuard::getEnum(DocId doc) const {
+    return _target_attribute->getEnum(getReferencedLid(doc));
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, largeint_t *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, double *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, const char **buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, EnumHandle *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedInt *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedFloat *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedString *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedConstChar *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedEnum *buffer, uint32_t sz) const {
+    return _target_attribute->get(getReferencedLid(docId), buffer, sz);
+}
+
+}
+}
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
new file mode 100644
index 00000000000..81a3d24b6cf
--- /dev/null
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
@@ -0,0 +1,53 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include "imported_attribute_vector.h"
+#include "attributeguard.h"
+
+namespace search::attribute {
+
+/*
+ * Short lived attribute vector that does not store values on its own.
+ *
+ * Extra information for direct lid to referenced lid mapping with
+ * boundary check is setup during construction.
+ */
+class ImportedAttributeVectorReadGuard : public ImportedAttributeVector
+{
+    using ReferencedLids = vespalib::ConstArrayRef<uint32_t>;
+    ReferencedLids                      _referencedLids;
+    uint32_t                            _referencedLidLimit;
+    AttributeGuard                      _reference_attribute_guard;
+    AttributeGuard                      _target_attribute_guard;
+    AttributeEnumGuard                  _target_attribute_enum_guard;
+
+    uint32_t getReferencedLid(uint32_t lid) const {
+        uint32_t referencedLid = _referencedLids[lid];
+        return ((referencedLid >= _referencedLidLimit) ? 0u : referencedLid);
+    }
+
+public:
+    ImportedAttributeVectorReadGuard(vespalib::stringref name,
+                                     std::shared_ptr<ReferenceAttribute> reference_attribute,
+                                     std::shared_ptr<AttributeVector> target_attribute,
+                                     bool stableEnumGuard);
+    ~ImportedAttributeVectorReadGuard();
+
+    virtual uint32_t getValueCount(uint32_t doc) const override;
+    virtual largeint_t getInt(DocId doc) const override;
+    virtual double getFloat(DocId doc) const override;
+    virtual const char *getString(DocId doc, char *buffer, size_t sz) const override;
+    virtual EnumHandle getEnum(DocId doc) const override;
+    virtual uint32_t get(DocId docId, largeint_t *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, double *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, const char **buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, EnumHandle *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, WeightedInt *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, WeightedFloat *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, WeightedString *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, WeightedConstChar *buffer, uint32_t sz) const override;
+    virtual uint32_t get(DocId docId, WeightedEnum *buffer, uint32_t sz) const override;
+};
+
+}