Merge pull request #26768 from vespa-engine/toregge/use-default-value-ref-from-enum-store-to-handle-cleardocv8.154.35

Use default value ref from enum store to handle CLEARDOC.

9 files changed, 17 insertions, 44 deletions

diff --git a/searchlib/src/vespa/searchlib/attribute/enumattribute.h b/searchlib/src/vespa/searchlib/attribute/enumattribute.h
index 773e5451703..4753dbe65f9 100644
--- a/searchlib/src/vespa/searchlib/attribute/enumattribute.h
+++ b/searchlib/src/vespa/searchlib/attribute/enumattribute.h
@@ -56,7 +56,6 @@ protected:
     virtual void considerAttributeChange(const Change & c, EnumStoreBatchUpdater & inserter) = 0;
     vespalib::MemoryUsage getEnumStoreValuesMemoryUsage() const override;
     void populate_address_space_usage(AddressSpaceUsage& usage) const override;
-    void cache_change_data_entry_ref(const Change& c) const;
 public:
     EnumAttribute(const vespalib::string & baseFileName, const AttributeVector::Config & cfg);
     ~EnumAttribute();
diff --git a/searchlib/src/vespa/searchlib/attribute/enumattribute.hpp b/searchlib/src/vespa/searchlib/attribute/enumattribute.hpp
index f0f518f64f7..66d555df3cb 100644
--- a/searchlib/src/vespa/searchlib/attribute/enumattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/enumattribute.hpp
@@ -86,13 +86,4 @@ EnumAttribute<B>::populate_address_space_usage(AddressSpaceUsage& usage) const
     usage.set(AddressSpaceComponents::enum_store, _enumStore.get_values_address_space_usage());
 }
 
-template <typename B>
-void
-EnumAttribute<B>::cache_change_data_entry_ref(const Change& c) const
-{
-    EnumIndex new_idx;
-    _enumStore.find_index(c._data.raw(), new_idx);
-    c.set_entry_ref(new_idx.ref());
-}
-
 } // namespace search
diff --git a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.h b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.h
index aac9a7b5416..7f36238ec6a 100644
--- a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.h
+++ b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.h
@@ -67,14 +67,14 @@ protected:
     void considerAttributeChange(const Change & c, EnumStoreBatchUpdater & inserter) override;
 
     // implemented by single value numeric enum attribute.
-    virtual void considerUpdateAttributeChange(const Change & c) { (void) c; }
+    virtual void considerUpdateAttributeChange(DocId, const Change&) { }
     virtual void considerArithmeticAttributeChange(const Change & c, EnumStoreBatchUpdater & inserter) { (void) c; (void) inserter; }
 
     virtual void applyValueChanges(EnumStoreBatchUpdater& updater) ;
     virtual void applyArithmeticValueChange(const Change& c, EnumStoreBatchUpdater& updater) {
         (void) c; (void) updater;
     }
-    void updateEnumRefCounts(const Change& c, EnumIndex newIdx, EnumIndex oldIdx, EnumStoreBatchUpdater& updater);
+    void updateEnumRefCounts(DocId doc, EnumIndex newIdx, EnumIndex oldIdx, EnumStoreBatchUpdater& updater);
 
     virtual void freezeEnumDictionary() {
         this->getEnumStore().freeze_dictionary();
diff --git a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.hpp
index f4f2b777abd..95976609940 100644
--- a/searchlib/src/vespa/searchlib/attribute/singleenumattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/singleenumattribute.hpp
@@ -146,7 +146,7 @@ SingleValueEnumAttribute<B>::considerUpdateAttributeChange(const Change & c, Enu
     } else {
         c.set_entry_ref(idx.ref());
     }
-    considerUpdateAttributeChange(c); // for numeric
+    considerUpdateAttributeChange(c._doc, c); // for numeric
 }
 
 template <typename B>
@@ -158,9 +158,7 @@ SingleValueEnumAttribute<B>::considerAttributeChange(const Change & c, EnumStore
     } else if (c._type >= ChangeBase::ADD && c._type <= ChangeBase::DIV) {
         considerArithmeticAttributeChange(c, inserter); // for numeric
     } else if (c._type == ChangeBase::CLEARDOC) {
-        Change clearDoc(this->_defaultValue);
-        clearDoc._doc = c._doc;
-        considerUpdateAttributeChange(clearDoc, inserter);
+        considerUpdateAttributeChange(c._doc, this->_defaultValue);
     }
 }
 
@@ -175,7 +173,7 @@ SingleValueEnumAttribute<B>::applyUpdateValueChange(const Change& c, EnumStoreBa
     } else {
         this->_enumStore.find_index(c._data.raw(), newIdx);
     }
-    updateEnumRefCounts(c, newIdx, oldIdx, updater);
+    updateEnumRefCounts(c._doc, newIdx, oldIdx, updater);
 }
 
 template <typename B>
@@ -183,30 +181,26 @@ void
 SingleValueEnumAttribute<B>::applyValueChanges(EnumStoreBatchUpdater& updater)
 {
     ValueModifier valueGuard(this->getValueModifier());
-    // This avoids searching for the defaultValue in the enum store for each CLEARDOC in the change vector.
-    this->cache_change_data_entry_ref(this->_defaultValue);
     for (const auto& change : this->_changes.getInsertOrder()) {
         if (change._type == ChangeBase::UPDATE) {
             applyUpdateValueChange(change, updater);
         } else if (change._type >= ChangeBase::ADD && change._type <= ChangeBase::DIV) {
             applyArithmeticValueChange(change, updater);
         } else if (change._type == ChangeBase::CLEARDOC) {
-            Change clearDoc(this->_defaultValue);
-            clearDoc._doc = change._doc;
-            applyUpdateValueChange(clearDoc, updater);
+            EnumIndex oldIdx = _enumIndices[change._doc].load_relaxed();
+            EnumIndex newIdx = this->_enumStore.get_default_value_ref().load_relaxed();
+            updateEnumRefCounts(change._doc, newIdx, oldIdx, updater);
         }
     }
-    // We must clear the cached entry ref as the defaultValue might be located in another data buffer on later invocations.
-    this->_defaultValue.clear_entry_ref();
 }
 
 template <typename B>
 void
-SingleValueEnumAttribute<B>::updateEnumRefCounts(const Change& c, EnumIndex newIdx, EnumIndex oldIdx,
+SingleValueEnumAttribute<B>::updateEnumRefCounts(DocId doc, EnumIndex newIdx, EnumIndex oldIdx,
                                                  EnumStoreBatchUpdater& updater)
 {
     updater.inc_ref_count(newIdx);
-    _enumIndices[c._doc].store_release(newIdx);
+    _enumIndices[doc].store_release(newIdx);
     if (oldIdx.valid()) {
         updater.dec_ref_count(oldIdx);
     }
diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h
index 5b0e1c6131e..4eeb6ceda57 100644
--- a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h
+++ b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h
@@ -43,7 +43,7 @@ private:
 protected:
 
     // from SingleValueEnumAttribute
-    void considerUpdateAttributeChange(const Change & c) override;
+    void considerUpdateAttributeChange(DocId doc, const Change & c) override;
     void considerArithmeticAttributeChange(const Change & c, EnumStoreBatchUpdater & inserter) override;
     void applyArithmeticValueChange(const Change& c, EnumStoreBatchUpdater& updater) override;
 
diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.hpp
index 14bf9cdc9f0..b840a0516b2 100644
--- a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.hpp
@@ -15,9 +15,9 @@ namespace search {
 
 template <typename B>
 void
-SingleValueNumericEnumAttribute<B>::considerUpdateAttributeChange(const Change & c)
+SingleValueNumericEnumAttribute<B>::considerUpdateAttributeChange(DocId doc, const Change & c)
 {
-    _currDocValues[c._doc] = c._data.get();
+    _currDocValues[doc] = c._data.get();
 }
 
 template <typename B>
@@ -53,7 +53,7 @@ SingleValueNumericEnumAttribute<B>::applyArithmeticValueChange(const Change& c,
     T newValue = this->template applyArithmetic<T, typename Change::DataType>(get(c._doc), c._data.getArithOperand(), c._type);
     this->_enumStore.find_index(newValue, newIdx);
 
-    this->updateEnumRefCounts(c, newIdx, oldIdx, updater);
+    this->updateEnumRefCounts(c._doc, newIdx, oldIdx, updater);
 }
 
 template <typename B>
diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp
index de4a7157dae..e353d03a9e8 100644
--- a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp
@@ -89,8 +89,6 @@ SingleValueNumericPostingAttribute<B>::applyValueChanges(EnumStoreBatchUpdater&
     // used to make sure several arithmetic operations on the same document in a single commit works
     std::map<DocId, EnumIndex> currEnumIndices;
 
-    // This avoids searching for the defaultValue in the enum store for each CLEARDOC in the change vector.
-    this->cache_change_data_entry_ref(this->_defaultValue);
     for (const auto& change : this->_changes.getInsertOrder()) {
         auto enumIter = currEnumIndices.find(change._doc);
         EnumIndex oldIdx;
@@ -111,13 +109,9 @@ SingleValueNumericPostingAttribute<B>::applyValueChanges(EnumStoreBatchUpdater&
                 currEnumIndices[change._doc] = newIdx;
             }
         } else if(change._type == ChangeBase::CLEARDOC) {
-            Change clearDoc(this->_defaultValue);
-            clearDoc._doc = change._doc;
-            applyUpdateValueChange(clearDoc, enumStore, currEnumIndices);
+            currEnumIndices[change._doc] = enumStore.get_default_value_ref().load_relaxed();
         }
     }
-    // We must clear the cached entry ref as the defaultValue might be located in another data buffer on later invocations.
-    this->_defaultValue.clear_entry_ref();
 
     makePostingChange(enumStore.get_comparator(), currEnumIndices, changePost);
 
diff --git a/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp
index 1ec9b54a73b..5b5214f6d3e 100644
--- a/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp
@@ -98,8 +98,6 @@ SingleValueStringPostingAttributeT<B>::applyValueChanges(EnumStoreBatchUpdater&
     // used to make sure several arithmetic operations on the same document in a single commit works
     std::map<DocId, EnumIndex> currEnumIndices;
 
-    // This avoids searching for the defaultValue in the enum store for each CLEARDOC in the change vector.
-    this->cache_change_data_entry_ref(this->_defaultValue);
     for (const auto& change : this->_changes.getInsertOrder()) {
         auto enumIter = currEnumIndices.find(change._doc);
         EnumIndex oldIdx;
@@ -111,12 +109,9 @@ SingleValueStringPostingAttributeT<B>::applyValueChanges(EnumStoreBatchUpdater&
         if (change._type == ChangeBase::UPDATE) {
             applyUpdateValueChange(change, enumStore, currEnumIndices);
         } else if (change._type == ChangeBase::CLEARDOC) {
-            this->_defaultValue._doc = change._doc;
-            applyUpdateValueChange(this->_defaultValue, enumStore, currEnumIndices);
+            currEnumIndices[change._doc] = enumStore.get_default_value_ref().load_relaxed();
         }
     }
-    // We must clear the cached entry ref as the defaultValue might be located in another data buffer on later invocations.
-    this->_defaultValue.clear_entry_ref();
 
     makePostingChange(enumStore.get_folded_comparator(), dictionary, currEnumIndices, changePost);
 
diff --git a/searchlib/src/vespa/searchlib/attribute/stringbase.h b/searchlib/src/vespa/searchlib/attribute/stringbase.h
index 5c6bf3c6b6a..98a3316947b 100644
--- a/searchlib/src/vespa/searchlib/attribute/stringbase.h
+++ b/searchlib/src/vespa/searchlib/attribute/stringbase.h
@@ -62,7 +62,7 @@ protected:
     using ChangeVector = ChangeVectorT<Change>;
     using EnumEntryType = const char*;
     ChangeVector _changes;
-    Change _defaultValue;
+    const Change _defaultValue;
     bool onLoad(vespalib::Executor *executor) override;
 
     bool onLoadEnumerated(ReaderBase &attrReader);