Merge pull request #23670 from vespa-engine/revert-23648-balder/use-faster-hash_set_of_int-take-2

Revert "Revert "Revert "Use a hash_set<int32_t> to quickly check if a field i…"

6 files changed, 61 insertions, 70 deletions

diff --git a/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp b/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
index 641e9879c79..b326db47b5c 100644
--- a/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
@@ -362,7 +362,7 @@ struct MyAttributeWriter : public IAttributeWriter
         _updateLid = lid;
         for (const auto & fieldUpdate : upd.getUpdates()) {
             search::AttributeVector * attr = getWritableAttribute(fieldUpdate.getField().getName());
-            onUpdate.onUpdateField(fieldUpdate.getField(), attr);
+            onUpdate.onUpdateField(fieldUpdate.getField().getName(), attr);
         }
     }
     void update(SerialNum serialNum, const document::Document &doc, DocumentIdT lid, OnWriteDoneType) override {
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
index 021fc4717af..bb25b3da7be 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
@@ -764,9 +764,9 @@ AttributeWriter::update(SerialNum serialNum, const DocumentUpdate &upd, Document
 
     for (const auto &fupd : upd.getUpdates()) {
         LOG(debug, "Retrieving guard for attribute vector '%s'.", fupd.getField().getName().data());
-        auto found = _attrMap.find(fupd.getField().getName());
-        AttributeVector * attrp = (found != _attrMap.end()) ? found->second.attribute : nullptr;
-        onUpdate.onUpdateField(fupd.getField(), attrp);
+        auto itr = _attrMap.find(fupd.getField().getName());
+        AttributeVector * attrp = (itr != _attrMap.end()) ? itr->second.attribute : nullptr;
+        onUpdate.onUpdateField(fupd.getField().getName(), attrp);
         if (__builtin_expect(attrp == nullptr, false)) {
             LOG(spam, "Failed to find attribute vector %s", fupd.getField().getName().data());
             continue;
@@ -776,15 +776,16 @@ AttributeWriter::update(SerialNum serialNum, const DocumentUpdate &upd, Document
         if (__builtin_expect(attrp->getStatus().getLastSyncToken() >= serialNum, false)) {
             continue;
         }
-        if (found->second.use_two_phase_put_for_assign_updates && is_single_assign_update(fupd)) {
+        if (itr->second.use_two_phase_put_for_assign_updates &&
+                is_single_assign_update(fupd)) {
             auto prepare_task = std::make_unique<PreparePutTask>(serialNum, lid, *attrp, get_single_assign_update_field_value(fupd));
             auto complete_task = std::make_unique<CompletePutTask>(*prepare_task, onWriteDone);
             LOG(debug, "About to handle assign update as two phase put for docid %u in attribute vector '%s'",
                 lid, attrp->getName().c_str());
             _shared_executor.execute(CpuUsage::wrap(std::move(prepare_task), CpuUsage::Category::WRITE));
-            _attributeFieldWriter.executeTask(found->second.executor_id, std::move(complete_task));
+            _attributeFieldWriter.executeTask(itr->second.executor_id, std::move(complete_task));
         } else {
-            args[found->second.executor_id.getId()]->_updates.emplace_back(attrp, &fupd);
+            args[itr->second.executor_id.getId()]->_updates.emplace_back(attrp, &fupd);
             LOG(debug, "About to apply update for docId %u in attribute vector '%s'.", lid, attrp->getName().c_str());
         }
     }
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/ifieldupdatecallback.h b/searchcore/src/vespa/searchcore/proton/attribute/ifieldupdatecallback.h
index d3ab970fb39..d8872607b44 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/ifieldupdatecallback.h
+++ b/searchcore/src/vespa/searchcore/proton/attribute/ifieldupdatecallback.h
@@ -5,17 +5,16 @@
 #include <vespa/vespalib/stllike/string.h>
 
 namespace search { class AttributeVector; }
-namespace document { class Field; }
 
 namespace proton {
 
 struct IFieldUpdateCallback {
-    virtual ~IFieldUpdateCallback() = default;
-    virtual void onUpdateField(const document::Field & field, const search::AttributeVector * attr) = 0;
+    virtual ~IFieldUpdateCallback() { }
+    virtual void onUpdateField(vespalib::stringref fieldName, const search::AttributeVector * attr) = 0;
 };
 
 struct DummyFieldUpdateCallback : IFieldUpdateCallback {
-    void onUpdateField(const document::Field & , const search::AttributeVector *) override {}
+    void onUpdateField(vespalib::stringref, const search::AttributeVector *) override {}
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/proton/server/searchable_feed_view.cpp b/searchcore/src/vespa/searchcore/proton/server/searchable_feed_view.cpp
index 7a78b4ba82a..207f1d813d8 100644
--- a/searchcore/src/vespa/searchcore/proton/server/searchable_feed_view.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/searchable_feed_view.cpp
@@ -34,7 +34,7 @@ SearchableFeedView::SearchableFeedView(StoreOnlyFeedView::Context storeOnlyCtx,
                                        const FastAccessFeedView::Context &fastUpdateCtx, Context ctx)
     : Parent(std::move(storeOnlyCtx), params, fastUpdateCtx),
       _indexWriter(ctx._indexWriter),
-      _hasIndexedFields(getSchema()->getNumIndexFields() > 0)
+      _hasIndexedFields(_schema->getNumIndexFields() > 0)
 { }
 
 SearchableFeedView::~SearchableFeedView() = default;
diff --git a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
index d7c3246cb4b..a537742b79b 100644
--- a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
@@ -7,14 +7,14 @@
 #include "putdonecontext.h"
 #include "removedonecontext.h"
 #include "updatedonecontext.h"
+#include <vespa/document/datatype/documenttype.h>
+#include <vespa/document/fieldvalue/document.h>
+#include <vespa/document/repo/documenttyperepo.h>
 #include <vespa/searchcore/proton/attribute/ifieldupdatecallback.h>
 #include <vespa/searchcore/proton/common/feedtoken.h>
 #include <vespa/searchcore/proton/feedoperation/operations.h>
 #include <vespa/searchcore/proton/reference/i_gid_to_lid_change_handler.h>
 #include <vespa/searchcore/proton/reference/i_pending_gid_to_lid_changes.h>
-#include <vespa/document/repo/documenttyperepo.h>
-#include <vespa/document/datatype/documenttype.h>
-#include <vespa/document/fieldvalue/document.h>
 #include <vespa/vespalib/util/cpu_usage.h>
 #include <vespa/vespalib/util/destructor_callbacks.h>
 #include <vespa/vespalib/util/exceptions.h>
@@ -88,8 +88,8 @@ SummaryPutDoneContext::SummaryPutDoneContext(FeedToken token, IPendingLidTracker
 
 SummaryPutDoneContext::~SummaryPutDoneContext() = default;
 
-std::vector<document::GlobalId>
-getGidsToRemove(const IDocumentMetaStore &metaStore, const LidVectorContext::LidVector &lidsToRemove)
+std::vector<document::GlobalId> getGidsToRemove(const IDocumentMetaStore &metaStore,
+                                                const LidVectorContext::LidVector &lidsToRemove)
 {
     std::vector<document::GlobalId> gids;
     gids.reserve(lidsToRemove.size());
@@ -102,9 +102,8 @@ getGidsToRemove(const IDocumentMetaStore &metaStore, const LidVectorContext::Lid
     return gids;
 }
 
-void
-putMetaData(documentmetastore::IStore &meta_store, const DocumentId & doc_id,
-            const DocumentOperation &op, bool is_removed_doc)
+void putMetaData(documentmetastore::IStore &meta_store, const DocumentId & doc_id,
+                 const DocumentOperation &op, bool is_removed_doc)
 {
     documentmetastore::IStore::Result putRes(
             meta_store.put(doc_id.getGlobalId(), op.getBucketId(), op.getTimestamp(),
@@ -118,9 +117,8 @@ putMetaData(documentmetastore::IStore &meta_store, const DocumentId & doc_id,
     assert(op.getLid() == putRes._lid);
 }
 
-void
-removeMetaData(documentmetastore::IStore &meta_store, const GlobalId & gid, const DocumentId &doc_id,
-               const DocumentOperation &op, bool is_removed_doc)
+void removeMetaData(documentmetastore::IStore &meta_store, const GlobalId & gid, const DocumentId &doc_id,
+                    const DocumentOperation &op, bool is_removed_doc)
 {
     assert(meta_store.validLid(op.getPrevLid()));
     assert(is_removed_doc == op.getPrevMarkedAsRemoved());
@@ -149,37 +147,6 @@ moveMetaData(documentmetastore::IStore &meta_store, const DocumentId & doc_id, c
     meta_store.move(op.getPrevLid(), op.getLid(), op.get_prepare_serial_num());
 }
 
-class UpdateScope final : public IFieldUpdateCallback
-{
-private:
-    const vespalib::hash_set<int32_t> & _indexedFields;
-    bool _nonAttributeFields;
-public:
-    bool _hasIndexedFields;
-
-    UpdateScope(const vespalib::hash_set<int32_t> & indexedFields, const DocumentUpdate & upd);
-    bool hasIndexOrNonAttributeFields() const {
-        return _hasIndexedFields || _nonAttributeFields;
-    }
-    void onUpdateField(const document::Field & field, const search::AttributeVector * attr) override;
-};
-
-UpdateScope::UpdateScope(const vespalib::hash_set<int32_t> & indexedFields, const DocumentUpdate & upd)
-    : _indexedFields(indexedFields),
-      _nonAttributeFields(!upd.getFieldPathUpdates().empty()),
-      _hasIndexedFields(false)
-{}
-
-void
-UpdateScope::onUpdateField(const document::Field & field, const search::AttributeVector * attr) {
-    if (!_nonAttributeFields && (attr == nullptr || !attr->isUpdateableInMemoryOnly())) {
-        _nonAttributeFields = true;
-    }
-    if (!_hasIndexedFields && (_indexedFields.find(field.getId()) != _indexedFields.end())) {
-        _hasIndexedFields = true;
-    }
-}
-
 }  // namespace
 
 StoreOnlyFeedView::StoreOnlyFeedView(Context ctx, const PersistentParams &params)
@@ -193,20 +160,12 @@ StoreOnlyFeedView::StoreOnlyFeedView(Context ctx, const PersistentParams &params
       _pendingLidsForDocStore(),
       _pendingLidsForCommit(std::move(ctx._pendingLidsForCommit)),
       _schema(std::move(ctx._schema)),
-      _indexedFields(),
       _writeService(ctx._writeService),
       _params(params),
       _metaStore(_documentMetaStoreContext->get()),
       _gidToLidChangeHandler(ctx._gidToLidChangeHandler)
 {
     _docType = _repo->getDocumentType(_params._docTypeName.getName());
-    if (_schema && _docType) {
-        for (const auto &indexField : _schema->getIndexFields()) {
-            document::FieldPath fieldPath;
-            _docType->buildFieldPath(fieldPath, indexField.getName());
-            _indexedFields.insert(fieldPath.back().getFieldRef().getId());
-        }
-    }
 }
 
 StoreOnlyFeedView::~StoreOnlyFeedView() = default;
@@ -248,7 +207,7 @@ void
 StoreOnlyFeedView::putAttributes(SerialNum, Lid, const Document &, OnPutDoneType) {}
 
 void
-StoreOnlyFeedView::putIndexedFields(SerialNum, Lid, const std::shared_ptr<Document> &, OnOperationDoneType) {}
+StoreOnlyFeedView::putIndexedFields(SerialNum, Lid, const Document::SP &, OnOperationDoneType) {}
 
 void
 StoreOnlyFeedView::preparePut(PutOperation &putOp)
@@ -326,7 +285,7 @@ StoreOnlyFeedView::updateAttributes(SerialNum, Lid, const DocumentUpdate & upd,
                                     OnOperationDoneType, IFieldUpdateCallback & onUpdate)
 {
     for (const auto & fieldUpdate : upd.getUpdates()) {
-        onUpdate.onUpdateField(fieldUpdate.getField(), nullptr);
+        onUpdate.onUpdateField(fieldUpdate.getField().getName(), nullptr);
     }
 }
 
@@ -428,6 +387,22 @@ StoreOnlyFeedView::heartBeatSummary(SerialNum serialNum, DoneCallback onDone) {
             }));
 }
 
+StoreOnlyFeedView::UpdateScope::UpdateScope(const search::index::Schema & schema, const DocumentUpdate & upd)
+    : _schema(&schema),
+      _indexedFields(false),
+      _nonAttributeFields(!upd.getFieldPathUpdates().empty())
+{}
+
+void
+StoreOnlyFeedView::UpdateScope::onUpdateField(vespalib::stringref fieldName, const search::AttributeVector * attr) {
+    if (!_nonAttributeFields && (attr == nullptr || !attr->isUpdateableInMemoryOnly())) {
+        _nonAttributeFields = true;
+    }
+    if (!_indexedFields && _schema->isIndexField(fieldName)) {
+        _indexedFields = true;
+    }
+}
+
 void
 StoreOnlyFeedView::internalUpdate(FeedToken token, const UpdateOperation &updOp) {
     if ( ! updOp.getUpdate()) {
@@ -457,7 +432,7 @@ StoreOnlyFeedView::internalUpdate(FeedToken token, const UpdateOperation &updOp)
     }
 
     auto onWriteDone = createUpdateDoneContext(std::move(token), get_pending_lid_token(updOp), updOp.getUpdate());
-    UpdateScope updateScope(_indexedFields, upd);
+    UpdateScope updateScope(*_schema, upd);
     updateAttributes(serialNum, lid, upd, onWriteDone, updateScope);
 
     if (updateScope.hasIndexOrNonAttributeFields()) {
@@ -465,7 +440,7 @@ StoreOnlyFeedView::internalUpdate(FeedToken token, const UpdateOperation &updOp)
         FutureDoc futureDoc = promisedDoc.get_future().share();
         onWriteDone->setDocument(futureDoc);
         _pendingLidsForDocStore.waitComplete(lid);
-        if (updateScope._hasIndexedFields) {
+        if (updateScope._indexedFields) {
             updateIndexedFields(serialNum, lid, futureDoc, onWriteDone);
         }
         PromisedStream promisedStream;
diff --git a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
index 25a98da7ce7..2822aa70525 100644
--- a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
+++ b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
@@ -9,6 +9,7 @@
 #include "searchcontext.h"
 #include <vespa/searchcore/proton/common/pendinglidtracker.h>
 #include <vespa/searchcore/proton/common/doctypename.h>
+#include <vespa/searchcore/proton/attribute/ifieldupdatecallback.h>
 #include <vespa/searchcore/proton/common/feeddebugger.h>
 #include <vespa/searchcore/proton/documentmetastore/documentmetastore.h>
 #include <vespa/searchcore/proton/documentmetastore/documentmetastorecontext.h>
@@ -17,10 +18,9 @@
 #include <vespa/searchcore/proton/persistenceengine/resulthandler.h>
 #include <vespa/searchcorespi/index/ithreadingservice.h>
 #include <vespa/searchlib/query/base.h>
-#include <vespa/searchcore/proton/feedoperation/operations.h>
 #include <vespa/vespalib/util/threadstackexecutorbase.h>
-#include <vespa/vespalib/stllike/hash_set.h>
 #include <future>
+#include <vespa/searchcore/proton/feedoperation/operations.h>
 
 namespace vespalib { class IDestructorCallback; }
 
@@ -118,6 +118,22 @@ public:
         {}
     };
 
+protected:
+    class UpdateScope : public IFieldUpdateCallback
+    {
+    private:
+        const search::index::Schema *_schema;
+    public:
+        bool _indexedFields;
+        bool _nonAttributeFields;
+
+        UpdateScope(const search::index::Schema & schema, const DocumentUpdate & upd);
+        bool hasIndexOrNonAttributeFields() const {
+            return _indexedFields || _nonAttributeFields;
+        }
+        void onUpdateField(vespalib::stringref fieldName, const search::AttributeVector * attr) override;
+    };
+
 private:
     const ISummaryAdapter::SP                                _summaryAdapter;
     const IDocumentMetaStoreContext::SP                      _documentMetaStoreContext;
@@ -126,9 +142,9 @@ private:
     LidReuseDelayer                                          _lidReuseDelayer;
     PendingLidTracker                                        _pendingLidsForDocStore;
     std::shared_ptr<PendingLidTrackerBase>                   _pendingLidsForCommit;
-    const search::index::Schema::SP                          _schema;
-    vespalib::hash_set<int32_t>                              _indexedFields;
+
 protected:
+    const search::index::Schema::SP          _schema;
     searchcorespi::index::IThreadingService &_writeService;
     PersistentParams                         _params;
     IDocumentMetaStore                      &_metaStore;