Merge pull request #3295 from vespa-engine/toregge/keep-better-track-of-referenced-lids

Keep track of mapping from gid to referenced lid in reference attribute

8 files changed, 99 insertions, 51 deletions

diff --git a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
index 1b1bc1f8796..07fb14c809b 100644
--- a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
+++ b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
@@ -188,9 +188,13 @@ struct Fixture
     void notifyReferencedRemove(const GlobalId &gid) {
         _attr->notifyReferencedRemove(gid);
     }
-    void populateReferencedLids() {
+    void setGidToLidMapperFactory(std::shared_ptr<MyGidToLidMapperFactory> factory) {
+        _attr->setGidToLidMapperFactory(factory);
         _attr->populateReferencedLids();
     }
+    uint32_t getUniqueGids() {
+        return getStatus().getNumUniqueValues();
+    }
 };
 
 TEST_F("require that we can instantiate reference attribute", Fixture)
@@ -289,8 +293,8 @@ TEST_F("require that we can save and load attribute", Fixture)
 TEST_F("require that update() uses gid-mapper to set referenced lid", Fixture)
 {
     f.ensureDocIdLimit(6);
-    std::shared_ptr<search::IGidToLidMapperFactory> factory = std::make_shared<MyGidToLidMapperFactory>();
-    f._attr->setGidToLidMapperFactory(factory);
+    auto factory = std::make_shared<MyGidToLidMapperFactory>();
+    f.setGidToLidMapperFactory(factory);
     f.set(1, toGid(doc1));
     f.set(2, toGid(doc2));
     f.set(4, toGid(doc1));
@@ -340,9 +344,8 @@ void preparePopulateReferencedLids(Fixture &f)
 
 void checkPopulateReferencedLids(Fixture &f)
 {
-    std::shared_ptr<search::IGidToLidMapperFactory> factory = std::make_shared<MyGidToLidMapperFactory>();
-    f._attr->setGidToLidMapperFactory(factory);
-    f.populateReferencedLids();
+    auto factory = std::make_shared<MyGidToLidMapperFactory>();
+    f.setGidToLidMapperFactory(factory);
     TEST_DO(f.assertRefLid(1, 10));
     TEST_DO(f.assertRefLid(2, 17));
     TEST_DO(f.assertRefLid(3, 10));
@@ -388,33 +391,35 @@ TEST_F("Require that notifyReferencedPut and notifyReferencedRemove changes reve
     TEST_DO(f.assertLids(11, { }));
 }
 
-TEST_F("Require that reverse mapping recovers from temporary out of order glitch", Fixture)
+TEST_F("Require that we track unique gids", Fixture)
 {
-    auto factory = std::make_shared<MyGidToLidMapperFactory>();
-    f._attr->setGidToLidMapperFactory(factory);
-    f.ensureDocIdLimit(4);
+    EXPECT_EQUAL(0u, f.getUniqueGids());
+    f.notifyReferencedPut(toGid(doc1), 10);
+    EXPECT_EQUAL(1u, f.getUniqueGids());
+    f.ensureDocIdLimit(3);
     f.set(1, toGid(doc1));
-    f.set(2, toGid(doc2));
-    /*
-     * Changes in gid to lid mapping can be visible via gid to lid
-     * mapper before notifications arrive.  If a lid is reused in the
-     * referenced document meta store then multiple entries in
-     * reference store might temporarily map to the same referenced
-     * lid.
-     */
-    factory->remove(doc1);   // remove referenced document
-    factory->add(doc3, 10);  // reuse lid for new referenced document
-    f.set(3, toGid(doc3));
+    f.commit();
+    EXPECT_EQUAL(1u, f.getUniqueGids());
     TEST_DO(f.assertRefLid(1, 10));
+    TEST_DO(f.assertLids(10, { 1 }));
+    f.set(2, toGid(doc2));
+    f.commit();
+    EXPECT_EQUAL(2u, f.getUniqueGids());
+    TEST_DO(f.assertRefLid(2, 0));
+    f.notifyReferencedPut(toGid(doc2), 17);
+    EXPECT_EQUAL(2u, f.getUniqueGids());
     TEST_DO(f.assertRefLid(2, 17));
-    TEST_DO(f.assertRefLid(3, 10));
-    // Notify reference attribute about gid to lid mapping changes
+    TEST_DO(f.assertLids(17, { 2 }));
+    f.clear(1);
+    f.notifyReferencedRemove(toGid(doc2));
+    EXPECT_EQUAL(2u, f.getUniqueGids());
+    TEST_DO(f.assertNoRefLid(1));
+    TEST_DO(f.assertRefLid(2, 0));
+    TEST_DO(f.assertLids(10, { }));
+    TEST_DO(f.assertLids(17, { }));
+    f.clear(2);
     f.notifyReferencedRemove(toGid(doc1));
-    f.notifyReferencedPut(toGid(doc3), 10);
-    TEST_DO(f.assertRefLid(1, 0));
-    TEST_DO(f.assertRefLid(2, 17));
-    TEST_DO(f.assertRefLid(3, 10));
-    TEST_DO(f.assertLids(10, { 3 }));
+    EXPECT_EQUAL(0u, f.getUniqueGids());
 }
 
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp b/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp
index 2cb05b14503..8666f0bdced 100644
--- a/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp
@@ -247,9 +247,6 @@ ReferenceAttribute::update(DocId doc, const GlobalId &gid)
     assert(doc < _indices.size());
     EntryRef oldRef = _indices[doc];
     Reference refToAdd(gid);
-    if (_gidToLidMapperFactory) {
-        refToAdd.setLid(_gidToLidMapperFactory->getMapper()->mapGidToLid(gid));
-    }
     EntryRef newRef = _store.add(refToAdd).ref();
     std::atomic_thread_fence(std::memory_order_release);
     _indices[doc] = newRef;
@@ -320,14 +317,23 @@ ReferenceAttribute::setGidToLidMapperFactory(std::shared_ptr<IGidToLidMapperFact
 }
 
 void
-ReferenceAttribute::notifyReferencedPut(const GlobalId &gid, DocId referencedLid)
+ReferenceAttribute::notifyReferencedPutNoCommit(const GlobalId &gid, DocId referencedLid)
 {
+    assert(referencedLid != 0);
     EntryRef ref = _store.find(gid);
-    if (ref.valid()) {
-        const auto &entry = _store.get(ref);
-        _referenceMappings.notifyReferencedPut(entry, referencedLid);
-        commit();
+    if (!ref.valid() || _store.get(ref).lid() == 0) {
+        Reference refToAdd(gid);
+        ref = _store.add(refToAdd).ref();
     }
+    const auto &entry = _store.get(ref);
+    _referenceMappings.notifyReferencedPut(entry, referencedLid);
+}
+
+void
+ReferenceAttribute::notifyReferencedPut(const GlobalId &gid, DocId referencedLid)
+{
+    notifyReferencedPutNoCommit(gid, referencedLid);
+    commit();
 }
 
 void
@@ -336,24 +342,42 @@ ReferenceAttribute::notifyReferencedRemove(const GlobalId &gid)
     EntryRef ref = _store.find(gid);
     if (ref.valid()) {
         const auto &entry = _store.get(ref);
+        uint32_t oldReferencedLid = entry.lid();
         _referenceMappings.notifyReferencedRemove(entry);
+        if (oldReferencedLid != 0) {
+            _store.remove(ref);
+        }
         commit();
     }
 }
 
+namespace {
+
+class ReferencedLidPopulator : public IGidToLidMapperVisitor
+{
+    ReferenceAttribute &_attr;
+public:
+    ReferencedLidPopulator(ReferenceAttribute &attr)
+        : IGidToLidMapperVisitor(),
+          _attr(attr)
+    {
+    }
+    virtual ~ReferencedLidPopulator() override { }
+    virtual void visit(const document::GlobalId &gid, uint32_t lid) const override {
+        _attr.notifyReferencedPutNoCommit(gid, lid);
+    }
+};
+
+}
+
 void
 ReferenceAttribute::populateReferencedLids()
 {
     if (_gidToLidMapperFactory) {
         std::unique_ptr<IGidToLidMapper> mapperUP = _gidToLidMapperFactory->getMapper();
         const IGidToLidMapper &mapper = *mapperUP;
-        const auto &store = _store;
-        const auto saver = _store.getSaver();
-        saver.foreach_key([&store,&mapper,this](EntryRef ref)
-                          {   const Reference &entry = store.get(ref);
-                              entry.setLid(mapper.mapGidToLid(entry.gid()));
-                              _referenceMappings.syncMappings(entry);
-                          });
+        ReferencedLidPopulator populator(*this);
+        mapper.foreach(populator);
     }
     commit();
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/reference_attribute.h b/searchlib/src/vespa/searchlib/attribute/reference_attribute.h
index ce7c908db99..7387c13fc50 100644
--- a/searchlib/src/vespa/searchlib/attribute/reference_attribute.h
+++ b/searchlib/src/vespa/searchlib/attribute/reference_attribute.h
@@ -78,6 +78,7 @@ public:
     ReverseMappingRefs getReverseMappingRefs() const { return _referenceMappings.getReverseMappingRefs(); }
     const ReverseMapping &getReverseMapping() const { return _referenceMappings.getReverseMapping(); }
 
+    void notifyReferencedPutNoCommit(const GlobalId &gid, DocId referencedLid);
     void notifyReferencedPut(const GlobalId &gid, DocId referencedLid);
     void notifyReferencedRemove(const GlobalId &gid);
     void populateReferencedLids();
diff --git a/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp b/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp
index f2462cdc40d..ae93dfba225 100644
--- a/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp
@@ -54,13 +54,6 @@ ReferenceMappings::syncReverseMappingIndices(const Reference &entry)
 }
 
 void
-ReferenceMappings::syncMappings(const Reference &entry)
-{
-    syncReverseMappingIndices(entry);
-    syncForwardMapping(entry);
-}
-
-void
 ReferenceMappings::removeReverseMapping(const Reference &entry, uint32_t lid)
 {
     EntryRef revMapIdx = entry.revMapIdx();
diff --git a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
index 73754d9cb13..43ae8a5bdb8 100644
--- a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
+++ b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
@@ -61,7 +61,6 @@ public:
     void notifyReferencedRemove(const Reference &entry);
     void removeReverseMapping(const Reference &entry, uint32_t lid);
     void addReverseMapping(const Reference &entry, uint32_t lid);
-    void syncMappings(const Reference &entry);
 
     // Maintain size of mapping from lid to referenced lid
     void onAddDocs(uint32_t docIdLimit);
diff --git a/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h b/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h
index f5317035a21..8a9e2dbd84f 100644
--- a/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h
+++ b/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h
@@ -3,12 +3,24 @@
 #pragma once
 
 #include <cstdint>
+#include <functional>
 
 namespace document { class GlobalId; }
 
 namespace search {
 
 /*
+ * Interface for gid to lid mapper visitor.
+ */
+class IGidToLidMapperVisitor
+{
+public:
+    virtual ~IGidToLidMapperVisitor() { }
+    virtual void visit(const document::GlobalId &gid, uint32_t lid) const = 0;
+};
+
+
+/*
  * Interface class for mapping from gid to lid. Instances should be short
  * lived due to implementations containing read guards preventing resource
  * reuse.
@@ -18,6 +30,7 @@ class IGidToLidMapper
 public:
     virtual ~IGidToLidMapper() { }
     virtual uint32_t mapGidToLid(const document::GlobalId &gid) const = 0;
+    virtual void foreach(const IGidToLidMapperVisitor &visitor) const = 0;
 };
 
 } // namespace search
diff --git a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
index edb1a789284..85a2b140039 100644
--- a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
+++ b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
@@ -101,6 +101,11 @@ struct ImportedAttributeFixture {
     void map_reference(DocId from_lid, GlobalId via_gid, DocId to_lid) {
         assert(from_lid < reference_attr->getNumDocs());
         mapper_factory->_map[via_gid] = to_lid;
+        if (to_lid != 0) {
+            reference_attr->notifyReferencedPut(via_gid, to_lid);
+        } else {
+            reference_attr->notifyReferencedRemove(via_gid);
+        }
         reference_attr->update(from_lid, via_gid);
         reference_attr->commit();
     }
diff --git a/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h b/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h
index c2ffc2c4dfd..86203c19970 100644
--- a/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h
+++ b/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h
@@ -29,6 +29,14 @@ struct MockGidToLidMapper : public search::IGidToLidMapper {
             return 0u;
         }
     }
+
+    void foreach(const search::IGidToLidMapperVisitor &visitor) const override {
+        for (const auto &kv : _map) {
+            if (kv.second != 0) {
+                visitor.visit(kv.first, kv.second);
+            }
+        }
+    }
 };
 
 struct MockGidToLidMapperFactory : public search::IGidToLidMapperFactory {