Merge pull request #20391 from vespa-engine/toregge/use-entry-ref-filter-for-remapping-after-compacting-dictionary-keys

Use EntryRefFilter to filter calls to UniqueStoreRemapper::remap() for

6 files changed, 49 insertions, 54 deletions

diff --git a/searchlib/src/vespa/searchlib/attribute/multienumattribute.cpp b/searchlib/src/vespa/searchlib/attribute/multienumattribute.cpp
index b114a355bb4..8790bdd9885 100644
--- a/searchlib/src/vespa/searchlib/attribute/multienumattribute.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/multienumattribute.cpp
@@ -30,13 +30,17 @@ remap_enum_store_refs(const EnumIndexRemapper& remapper, AttributeVector& v, att
     v.logEnumStoreEvent("compactfixup", "drain");
     {
         AttributeVector::EnumModifier enum_guard(v.getEnumModifier());
+        auto& filter = remapper.get_entry_ref_filter();
         v.logEnumStoreEvent("compactfixup", "start");
         for (uint32_t doc = 0; doc < v.getNumDocs(); ++doc) {
             vespalib::ConstArrayRef<WeightedIndex> indicesRef(multi_value_mapping.get(doc));
             WeightedIndexVector indices(indicesRef.cbegin(), indicesRef.cend());
             for (uint32_t i = 0; i < indices.size(); ++i) {
-                EnumIndex oldIndex = indices[i].value();
-                indices[i] = WeightedIndex(remapper.remap(oldIndex), indices[i].weight());
+                EnumIndex ref = indices[i].value();
+                if (ref.valid() && filter.has(ref)) {
+                    ref = remapper.remap(ref);
+                }
+                indices[i] = WeightedIndex(ref, indices[i].weight());
             }
             std::atomic_thread_fence(std::memory_order_release);
             multi_value_mapping.replace(doc, indices);
diff --git a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.cpp b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.cpp
index 4323e57f6b1..18805a7b20f 100644
--- a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.cpp
@@ -49,13 +49,16 @@ SingleValueEnumAttributeBase::remap_enum_store_refs(const EnumIndexRemapper& rem
 {
     // update _enumIndices with new EnumIndex values after enum store has been compacted.
     v.logEnumStoreEvent("reenumerate", "reserved");
-    auto new_indexes = std::make_unique<vespalib::Array<EnumIndex>>();
-    new_indexes->reserve(_enumIndices.capacity());
+    vespalib::Array<EnumIndex> new_indexes;
+    new_indexes.reserve(_enumIndices.capacity());
     v.logEnumStoreEvent("reenumerate", "start");
+    auto& filter = remapper.get_entry_ref_filter();
     for (uint32_t i = 0; i < _enumIndices.size(); ++i) {
-        EnumIndex old_index = _enumIndices[i];
-        EnumIndex new_index = remapper.remap(old_index);
-        new_indexes->push_back_fast(new_index);
+        EnumIndex ref = _enumIndices[i];
+        if (ref.valid() && filter.has(ref)) {
+            ref = remapper.remap(ref);
+        }
+        new_indexes.push_back_fast(ref);
     }
     v.logEnumStoreEvent("compactfixup", "drain");
     {
diff --git a/vespalib/src/tests/util/rcuvector/rcuvector_test.cpp b/vespalib/src/tests/util/rcuvector/rcuvector_test.cpp
index 9ad0e95667b..cf84ab03a25 100644
--- a/vespalib/src/tests/util/rcuvector/rcuvector_test.cpp
+++ b/vespalib/src/tests/util/rcuvector/rcuvector_test.cpp
@@ -19,19 +19,14 @@ assertUsage(const MemoryUsage & exp, const MemoryUsage & act)
 
 TEST("test generation holder")
 {
-    typedef std::unique_ptr<int32_t> IntPtr;
     GenerationHolder gh;
-    gh.hold(GenerationHeldBase::UP(new RcuVectorHeld<int32_t>(sizeof(int32_t),
-                                           IntPtr(new int32_t(0)))));
+    gh.hold(std::make_unique<RcuVectorHeld<int32_t>>(sizeof(int32_t), 0));
     gh.transferHoldLists(0);
-    gh.hold(GenerationHeldBase::UP(new RcuVectorHeld<int32_t>(sizeof(int32_t),
-                                           IntPtr(new int32_t(1)))));
+    gh.hold(std::make_unique<RcuVectorHeld<int32_t>>(sizeof(int32_t), 1));
     gh.transferHoldLists(1);
-    gh.hold(GenerationHeldBase::UP(new RcuVectorHeld<int32_t>(sizeof(int32_t),
-                                           IntPtr(new int32_t(2)))));
+    gh.hold(std::make_unique<RcuVectorHeld<int32_t>>(sizeof(int32_t), 2));
     gh.transferHoldLists(2);
-    gh.hold(GenerationHeldBase::UP(new RcuVectorHeld<int32_t>(sizeof(int32_t),
-                                           IntPtr(new int32_t(4)))));
+    gh.hold(std::make_unique<RcuVectorHeld<int32_t>>(sizeof(int32_t), 4));
     gh.transferHoldLists(4);
     EXPECT_EQUAL(4u * sizeof(int32_t), gh.getHeldBytes());
     gh.trimHoldLists(0);
@@ -40,8 +35,7 @@ TEST("test generation holder")
     EXPECT_EQUAL(3u * sizeof(int32_t), gh.getHeldBytes());
     gh.trimHoldLists(2);
     EXPECT_EQUAL(2u * sizeof(int32_t), gh.getHeldBytes());
-    gh.hold(GenerationHeldBase::UP(new RcuVectorHeld<int32_t>(sizeof(int32_t),
-                                       IntPtr(new int32_t(6)))));
+    gh.hold(std::make_unique<RcuVectorHeld<int32_t>>(sizeof(int32_t), 6));
     gh.transferHoldLists(6);
     EXPECT_EQUAL(3u * sizeof(int32_t), gh.getHeldBytes());
     gh.trimHoldLists(6);
diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h b/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h
index 873af07a902..2501c4fafd9 100644
--- a/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h
+++ b/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h
@@ -30,32 +30,24 @@ public:
     virtual ~UniqueStoreRemapper() = default;
 
     EntryRef remap(EntryRef ref) const {
-        if (ref.valid()) {
-            if (!_compacting_buffer.has(ref)) {
-                // No remapping for references to buffers not being compacted
-                return ref;
-            } else {
-                RefType internal_ref(ref);
-                auto &inner_mapping = _mapping[internal_ref.bufferId()];
-                assert(internal_ref.unscaled_offset() < inner_mapping.size());
-                EntryRef mapped_ref = inner_mapping[internal_ref.unscaled_offset()];
-                assert(mapped_ref.valid());
-                return mapped_ref;
-            }
-        } else {
-            return EntryRef();
-        }
+        RefType internal_ref(ref);
+        auto &inner_mapping = _mapping[internal_ref.bufferId()];
+        assert(internal_ref.unscaled_offset() < inner_mapping.size());
+        EntryRef mapped_ref = inner_mapping[internal_ref.unscaled_offset()];
+        assert(mapped_ref.valid());
+        return mapped_ref;
     }
 
     void remap(vespalib::ArrayRef<EntryRef> refs) const {
         for (auto &ref : refs) {
-            auto mapped_ref = remap(ref);
-            if (mapped_ref != ref) {
-                ref = mapped_ref;
+            if (ref.valid() && _compacting_buffer.has(ref)) {
+                ref = remap(ref);
             }
         }
     }
 
+    const EntryRefFilter& get_entry_ref_filter() const noexcept { return _compacting_buffer; }
+
     virtual void done() = 0;
 };
 
diff --git a/vespalib/src/vespa/vespalib/util/rcuvector.h b/vespalib/src/vespa/vespalib/util/rcuvector.h
index 0396ee0d459..dd4fa660279 100644
--- a/vespalib/src/vespa/vespalib/util/rcuvector.h
+++ b/vespalib/src/vespa/vespalib/util/rcuvector.h
@@ -13,10 +13,10 @@ namespace vespalib {
 template <typename T>
 class RcuVectorHeld : public GenerationHeldBase
 {
-    std::unique_ptr<T> _data;
+    T _data;
 
 public:
-    RcuVectorHeld(size_t size, std::unique_ptr<T> data);
+    RcuVectorHeld(size_t size, T&& data);
 
     ~RcuVectorHeld();
 };
@@ -121,7 +121,7 @@ public:
 
     void reset();
     void shrink(size_t newSize) __attribute__((noinline));
-    void replaceVector(std::unique_ptr<ArrayType> replacement);
+    void replaceVector(ArrayType replacement);
 };
 
 template <typename T>
diff --git a/vespalib/src/vespa/vespalib/util/rcuvector.hpp b/vespalib/src/vespa/vespalib/util/rcuvector.hpp
index 9d7c8ea57d6..3c455149dfd 100644
--- a/vespalib/src/vespa/vespalib/util/rcuvector.hpp
+++ b/vespalib/src/vespa/vespalib/util/rcuvector.hpp
@@ -9,7 +9,7 @@
 namespace vespalib {
 
 template <typename T>
-RcuVectorHeld<T>::RcuVectorHeld(size_t size, std::unique_ptr<T> data)
+RcuVectorHeld<T>::RcuVectorHeld(size_t size, T&& data)
     : GenerationHeldBase(size),
       _data(std::move(data))
 { }
@@ -52,20 +52,21 @@ RcuVectorBase<T>::~RcuVectorBase() = default;
 template <typename T>
 void
 RcuVectorBase<T>::expand(size_t newCapacity) {
-    std::unique_ptr<ArrayType> tmpData(new ArrayType());
-    tmpData->reserve(newCapacity);
+    ArrayType tmpData;
+    tmpData.reserve(newCapacity);
     for (const T & v : _data) {
-        tmpData->push_back_fast(v);
+        tmpData.push_back_fast(v);
     }
     replaceVector(std::move(tmpData));
 }
 
 template <typename T>
 void
-RcuVectorBase<T>::replaceVector(std::unique_ptr<ArrayType> replacement) {
-    replacement->swap(_data); // atomic switch of underlying data
-    size_t holdSize = replacement->capacity() * sizeof(T);
-    GenerationHeldBase::UP hold(new RcuVectorHeld<ArrayType>(holdSize, std::move(replacement)));
+RcuVectorBase<T>::replaceVector(ArrayType replacement) {
+    std::atomic_thread_fence(std::memory_order_release);
+    replacement.swap(_data); // atomic switch of underlying data
+    size_t holdSize = replacement.capacity() * sizeof(T);
+    auto hold = std::make_unique<RcuVectorHeld<ArrayType>>(holdSize, std::move(replacement));
     _genHolder.hold(std::move(hold));
     onReallocation();
 }
@@ -90,17 +91,18 @@ RcuVectorBase<T>::shrink(size_t newSize)
         return;
     }
     if (!_data.try_unreserve(wantedCapacity)) {
-        std::unique_ptr<ArrayType> tmpData(new ArrayType());
-        tmpData->reserve(wantedCapacity);
-        tmpData->resize(newSize);
+        ArrayType tmpData;
+        tmpData.reserve(wantedCapacity);
+        tmpData.resize(newSize);
         for (uint32_t i = 0; i < newSize; ++i) {
-            (*tmpData)[i] = _data[i];
+            tmpData[i] = _data[i];
         }
+        std::atomic_thread_fence(std::memory_order_release);
         // Users of RCU vector must ensure that no readers use old size
         // after swap.  Attribute vectors uses _committedDocIdLimit for this.
-        tmpData->swap(_data); // atomic switch of underlying data
-        size_t holdSize = tmpData->capacity() * sizeof(T);
-        GenerationHeldBase::UP hold(new RcuVectorHeld<ArrayType>(holdSize, std::move(tmpData)));
+        tmpData.swap(_data); // atomic switch of underlying data
+        size_t holdSize = tmpData.capacity() * sizeof(T);
+        auto hold = std::make_unique<RcuVectorHeld<ArrayType>>(holdSize, std::move(tmpData));
         _genHolder.hold(std::move(hold));
         onReallocation();
     }