The AttributeContext is a short lived cache for attributes guards. Until we use the thread bundle

we do not need to use expensive locking to fill the cache.
Most of the attributes are pulled in when building the blueprint tree, and that always happens singlethreaded.

7 files changed, 69 insertions, 25 deletions

diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attributemanager.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attributemanager.cpp
index ee04f17d378..96d99008309 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attributemanager.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attributemanager.cpp
@@ -451,7 +451,7 @@ namespace {
 class CombinedAttributeContext : public IAttributeContext {
 private:
     using IAttributeFunctor = search::attribute::IAttributeFunctor;
-    AttributeContext _ctx;
+    AttributeContext          _ctx;
     ImportedAttributesContext _importedCtx;
 
 public:
@@ -483,6 +483,10 @@ public:
         _ctx.releaseEnumGuards();
         _importedCtx.releaseEnumGuards();
     }
+    void enableMultiThreadSafe() override {
+        _ctx.enableMultiThreadSafe();
+        _importedCtx.enableMultiThreadSafe();
+    }
     void asyncForAttribute(const vespalib::string &name, std::unique_ptr<IAttributeFunctor> func) const override {
         _ctx.asyncForAttribute(name, std::move(func));
     }
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
index cb05ac7e9f8..ede86051efb 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.cpp
@@ -14,8 +14,7 @@ using LockGuard = std::lock_guard<std::mutex>;
 namespace proton {
 
 const IAttributeVector *
-ImportedAttributesContext::getOrCacheAttribute(const vespalib::string &name, AttributeCache &attributes,
-                                               bool stableEnumGuard, const LockGuard &) const
+ImportedAttributesContext::getOrCacheAttribute(const vespalib::string &name, AttributeCache &attributes, bool stableEnumGuard) const
 {
     auto itr = attributes.find(name);
     if (itr != attributes.end()) {
@@ -38,8 +37,15 @@ ImportedAttributesContext::getOrCacheAttribute(const vespalib::string &name, Att
     }
 }
 
+const IAttributeVector *
+ImportedAttributesContext::getOrCacheAttributeMtSafe(const vespalib::string &name, AttributeCache &attributes, bool stableEnumGuard) const {
+    LockGuard guard(_cacheMutex);
+    return getOrCacheAttribute(name, attributes, stableEnumGuard);
+}
+
 ImportedAttributesContext::ImportedAttributesContext(const ImportedAttributesRepo &repo)
     : _repo(repo),
+      _mtSafe(false),
       _guardedAttributes(),
       _enumGuardedAttributes(),
       _metaStores(),
@@ -52,15 +58,17 @@ ImportedAttributesContext::~ImportedAttributesContext() = default;
 const IAttributeVector *
 ImportedAttributesContext::getAttribute(const vespalib::string &name) const
 {
-    LockGuard guard(_cacheMutex);
-    return getOrCacheAttribute(name, _guardedAttributes, false, guard);
+    return _mtSafe
+        ? getOrCacheAttributeMtSafe(name, _guardedAttributes, false)
+        : getOrCacheAttribute(name, _guardedAttributes, false);
 }
 
 const IAttributeVector *
 ImportedAttributesContext::getAttributeStableEnum(const vespalib::string &name) const
 {
-    LockGuard guard(_cacheMutex);
-    return getOrCacheAttribute(name, _enumGuardedAttributes, true, guard);
+    return _mtSafe
+           ? getOrCacheAttributeMtSafe(name, _enumGuardedAttributes, true)
+           : getOrCacheAttribute(name, _enumGuardedAttributes, true);
 }
 
 void
@@ -76,8 +84,12 @@ ImportedAttributesContext::getAttributeList(std::vector<const IAttributeVector *
 void
 ImportedAttributesContext::releaseEnumGuards()
 {
-    LockGuard guard(_cacheMutex);
-    _enumGuardedAttributes.clear();
+    if (_mtSafe) {
+        LockGuard guard(_cacheMutex);
+        _enumGuardedAttributes.clear();
+    } else {
+        _enumGuardedAttributes.clear();
+    }
 }
 
 void
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
index a90012a6ef8..aefffd959de 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
+++ b/searchcore/src/vespa/searchcore/proton/attribute/imported_attributes_context.h
@@ -38,13 +38,14 @@ private:
     using LockGuard = std::lock_guard<std::mutex>;
 
     const ImportedAttributesRepo &_repo;
-    mutable AttributeCache _guardedAttributes;
-    mutable AttributeCache _enumGuardedAttributes;
-    mutable MetaStoreCache _metaStores;
-    mutable std::mutex     _cacheMutex;
+    bool                          _mtSafe;
+    mutable AttributeCache        _guardedAttributes;
+    mutable AttributeCache        _enumGuardedAttributes;
+    mutable MetaStoreCache        _metaStores;
+    mutable std::mutex            _cacheMutex;
 
-    const IAttributeVector *getOrCacheAttribute(const vespalib::string &name, AttributeCache &attributes,
-                                                bool stableEnumGuard, const LockGuard &) const;
+    const IAttributeVector *getOrCacheAttribute(const vespalib::string &name, AttributeCache &attributes, bool stableEnumGuard) const;
+    const IAttributeVector *getOrCacheAttributeMtSafe(const vespalib::string &name, AttributeCache &attributes, bool stableEnumGuard) const;
 
 public:
     ImportedAttributesContext(const ImportedAttributesRepo &repo);
@@ -55,6 +56,7 @@ public:
     const IAttributeVector *getAttributeStableEnum(const vespalib::string &name) const override;
     void getAttributeList(std::vector<const IAttributeVector *> &list) const override;
     void releaseEnumGuards() override;
+    void enableMultiThreadSafe() override { _mtSafe = true; }
 
     void asyncForAttribute(const vespalib::string &name, std::unique_ptr<IAttributeFunctor> func) const override;
 };
diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
index e2c5f4ef559..d0c1f99af11 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
@@ -63,7 +63,8 @@ struct StupidMetaStore : search::IDocumentMetaStore {
     void foreach(const search::IGidToLidMapperVisitor &) const override { }
 };
 
-size_t numThreads(size_t hits, size_t minHits) {
+size_t
+numThreads(size_t hits, size_t minHits) {
     return static_cast<size_t>(std::ceil(double(hits) / double(minHits)));
 }
 
@@ -74,16 +75,17 @@ public:
         _threadBundle(threadBundle),
         _maxThreads(std::min(maxThreads, static_cast<uint32_t>(threadBundle.size())))
     { }
-private:
     size_t size() const override { return _maxThreads; }
     void run(vespalib::Runnable* const* targets, size_t cnt) override {
         _threadBundle.run(targets, cnt);
     }
+private:
     vespalib::ThreadBundle &_threadBundle;
     const uint32_t          _maxThreads;
 };
 
-bool willNotNeedRanking(const SearchRequest & request, const GroupingContext & groupingContext) {
+bool
+willNotNeedRanking(const SearchRequest & request, const GroupingContext & groupingContext) {
     return (!groupingContext.needRanking() && (request.maxhits == 0))
            || (!request.sortSpec.empty() && (request.sortSpec.find("[rank]") == vespalib::string::npos));
 }
@@ -221,6 +223,7 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl
         }
         const Properties *feature_overrides = &request.propertiesMap.featureOverrides();
         if (shouldCacheSearchSession) {
+            // These should have been moved instead.
             owned_objects.feature_overrides = std::make_unique<Properties>(*feature_overrides);
             feature_overrides = owned_objects.feature_overrides.get();
         }
@@ -248,6 +251,9 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl
         LimitedThreadBundleWrapper limitedThreadBundle(threadBundle, numThreadsPerSearch);
         MatchMaster master;
         uint32_t numParts = NumSearchPartitions::lookup(rankProperties, _rankSetup->getNumSearchPartitions());
+        if (limitedThreadBundle.size() > 1) {
+            attrContext.enableMultiThreadSafe();
+        }
         ResultProcessor::Result::UP result = master.match(request.trace(), params, limitedThreadBundle, *mtf, rp,
                                                           _distributionKey, numParts);
         my_stats = MatchMaster::getStats(std::move(master));
diff --git a/searchlib/src/vespa/searchcommon/attribute/iattributecontext.h b/searchlib/src/vespa/searchcommon/attribute/iattributecontext.h
index 9c89b6a0f8b..cf7b1d2f959 100644
--- a/searchlib/src/vespa/searchcommon/attribute/iattributecontext.h
+++ b/searchlib/src/vespa/searchcommon/attribute/iattributecontext.h
@@ -46,6 +46,11 @@ public:
     virtual void releaseEnumGuards() {}
 
     /**
+     * Must be called before multiple threads will access the context.
+     */
+    virtual void enableMultiThreadSafe() {}
+
+    /**
      * Virtual destructor to allow safe subclassing.
      **/
     virtual ~IAttributeContext() = default;
diff --git a/searchlib/src/vespa/searchlib/attribute/attributecontext.cpp b/searchlib/src/vespa/searchlib/attribute/attributecontext.cpp
index 3c85c076eb2..97a7dc8bcb1 100644
--- a/searchlib/src/vespa/searchlib/attribute/attributecontext.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/attributecontext.cpp
@@ -30,8 +30,15 @@ AttributeContext::getAttribute(AttributeMap & map, const string & name, bool sta
     }
 }
 
+const IAttributeVector *
+AttributeContext::getAttributeMtSafe(AttributeMap &map, const string &name, bool stableEnum) const {
+    std::lock_guard<std::mutex> guard(_cacheLock);
+    return getAttribute(map, name, stableEnum);
+}
+
 AttributeContext::AttributeContext(const IAttributeManager & manager)
     : _manager(manager),
+      _mtSafe(false),
       _attributes(),
       _enumAttributes(),
       _cacheLock()
@@ -42,20 +49,26 @@ AttributeContext::~AttributeContext() = default;
 const IAttributeVector *
 AttributeContext::getAttribute(const string & name) const
 {
-    std::lock_guard<std::mutex> guard(_cacheLock);
-    return getAttribute(_attributes, name, false);
+    return _mtSafe
+        ? getAttributeMtSafe(_attributes, name, false)
+        : getAttribute(_attributes, name, false);
 }
 
 const IAttributeVector *
 AttributeContext::getAttributeStableEnum(const string & name) const
 {
-    std::lock_guard<std::mutex> guard(_cacheLock);
-    return getAttribute(_enumAttributes, name, true);
+    return _mtSafe
+           ? getAttributeMtSafe(_enumAttributes, name, true)
+           : getAttribute(_enumAttributes, name, true);
 }
 
 void AttributeContext::releaseEnumGuards() {
-    std::lock_guard<std::mutex> guard(_cacheLock);
-    _enumAttributes.clear();
+    if (_mtSafe) {
+        std::lock_guard<std::mutex> guard(_cacheLock);
+        _enumAttributes.clear();
+    } else {
+        _enumAttributes.clear();
+    }
 }
 
 void
diff --git a/searchlib/src/vespa/searchlib/attribute/attributecontext.h b/searchlib/src/vespa/searchlib/attribute/attributecontext.h
index 8b8cf5daead..28b05a76f65 100644
--- a/searchlib/src/vespa/searchlib/attribute/attributecontext.h
+++ b/searchlib/src/vespa/searchlib/attribute/attributecontext.h
@@ -21,12 +21,13 @@ private:
     using IAttributeFunctor = attribute::IAttributeFunctor;
 
     const IAttributeManager & _manager;
+    bool                      _mtSafe;
     mutable AttributeMap      _attributes;
     mutable AttributeMap      _enumAttributes;
     mutable std::mutex        _cacheLock;
 
     const IAttributeVector *getAttribute(AttributeMap & map, const string & name, bool stableEnum) const;
-
+    const IAttributeVector *getAttributeMtSafe(AttributeMap & map, const string & name, bool stableEnum) const;
 public:
     AttributeContext(const IAttributeManager & manager);
     ~AttributeContext() override;
@@ -37,6 +38,7 @@ public:
     const attribute::IAttributeVector * getAttributeStableEnum(const string & name) const override;
     void getAttributeList(std::vector<const IAttributeVector *> & list) const override;
     void releaseEnumGuards() override;
+    void enableMultiThreadSafe() override { _mtSafe = true; }
 
     // Give acces to the underlying manager
     const IAttributeManager & getManager() const { return _manager; }