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

even when no documents currently reference that gid in the reference attribute.
This eliminates the need for using the gid to lid mapper after
populateReferencedLids() has been called.

8 files changed, 72 insertions, 58 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..480b58d672d 100644
--- a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
+++ b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
@@ -188,7 +188,8 @@ struct Fixture
     void notifyReferencedRemove(const GlobalId &gid) {
         _attr->notifyReferencedRemove(gid);
     }
-    void populateReferencedLids() {
+    void setGidToLidMapperFactory(std::shared_ptr<MyGidToLidMapperFactory> factory) {
+        _attr->setGidToLidMapperFactory(factory);
         _attr->populateReferencedLids();
     }
 };
@@ -289,8 +290,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 +341,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 +388,4 @@ 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)
-{
-    auto factory = std::make_shared<MyGidToLidMapperFactory>();
-    f._attr->setGidToLidMapperFactory(factory);
-    f.ensureDocIdLimit(4);
-    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));
-    TEST_DO(f.assertRefLid(1, 10));
-    TEST_DO(f.assertRefLid(2, 17));
-    TEST_DO(f.assertRefLid(3, 10));
-    // Notify reference attribute about gid to lid mapping changes
-    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 }));
-}
-
 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 {