From 124e3463fc437675983200639ffd00feb8524577 Mon Sep 17 00:00:00 2001
From: Geza Lore <gezalore@gmail.com>
Date: Tue, 13 Aug 2024 18:52:23 +0100
Subject: [PATCH] Fix cache config file resolution performance (#5369)

V3ConfigWildcardResolver::resolve used to only cache the result of a
name lookup if there were any directives applied to the queried name.

This can be a performance problem. E.g. the module resolver is invoked
for every variable to check for attributes. If you had 'n' variables in
the design, and you also had 'k' directives that apply to specific
modules not involving most of the 'n' variables, then checking the
attributes involved O(n*k) operations. This can be very expensive
for large 'n' and 'k'.

In this patch we resolve every name exactly once, so the same above is
now O(n) in the worst case, and often a lot smaller due to repeat
lookups.
---
 src/V3Config.cpp | 54 ++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/src/V3Config.cpp b/src/V3Config.cpp
index 47c34b045..7e518de80 100644
--- a/src/V3Config.cpp
+++ b/src/V3Config.cpp
@@ -20,6 +20,7 @@
 
 #include "V3String.h"
 
+#include <memory>
 #include <set>
 #include <unordered_map>
 
@@ -34,11 +35,12 @@ VL_DEFINE_DEBUG_FUNCTIONS;
 // function that takes a reference of this type to join multiple entities into one.
 template <typename T>
 class V3ConfigWildcardResolver final {
-    using Map = std::map<const std::string, T>;
-
     mutable V3Mutex m_mutex;  // protects members
-    Map m_mapWildcard VL_GUARDED_BY(m_mutex);  // Wildcard strings to entities
-    Map m_mapResolved VL_GUARDED_BY(m_mutex);  // Resolved strings to converged entities
+    // Pattern strings (wildcard, or simple name) to entities
+    std::map<const std::string, T> m_mapPatterns VL_GUARDED_BY(m_mutex);
+    // Resolved strings to converged entities - nullptr, iff none of the patterns applies
+    std::map<const std::string, std::unique_ptr<T>> m_mapResolved VL_GUARDED_BY(m_mutex);
+
 public:
     V3ConfigWildcardResolver() = default;
     ~V3ConfigWildcardResolver() = default;
@@ -48,41 +50,36 @@ public:
         VL_EXCLUDES(other.m_mutex) {
         V3LockGuard lock{m_mutex};
         V3LockGuard otherLock{other.m_mutex};
-        for (const auto& itr : other.m_mapResolved) m_mapResolved[itr.first].update(itr.second);
-        for (const auto& itr : other.m_mapWildcard) m_mapWildcard[itr.first].update(itr.second);
+        // Clear the resolved cache, as 'other' might add new patterns that need to be applied as
+        // well.
+        m_mapResolved.clear();
+        for (const auto& itr : other.m_mapPatterns) m_mapPatterns[itr.first].update(itr.second);
     }
 
-    // Access and create a (wildcard) entity
+    // Access and create a pattern entry
     T& at(const string& name) VL_MT_SAFE_EXCLUDES(m_mutex) {
         V3LockGuard lock{m_mutex};
-        // Don't store into wildcards if the name is not a wildcard string
-        return m_mapWildcard[name];
+        // We might be adding a new entry under this, so clear the cache.
+        m_mapResolved.clear();
+        return m_mapPatterns[name];
     }
-    // Access an entity and resolve wildcards that match it
+    // Access an entity and resolve patterns that match it
     T* resolve(const string& name) VL_MT_SAFE_EXCLUDES(m_mutex) {
         V3LockGuard lock{m_mutex};
         // Lookup if it was resolved before, typically not
-        auto it = m_mapResolved.find(name);
-        if (VL_UNLIKELY(it != m_mapResolved.end())) return &it->second;
-
-        T* newp = nullptr;
-        // Cannot be resolved, create if matched
-
-        // Update this entity with all matches in the wildcards
-        for (const auto& wildent : m_mapWildcard) {
-            if (VString::wildmatch(name, wildent.first)) {
-                if (!newp) {
-                    newp = &m_mapResolved[name];  // Emplace and get pointer
+        const auto pair = m_mapResolved.emplace(name, nullptr);
+        std::unique_ptr<T>& entryr = pair.first->second;
+        // Resolve entry when first requested, cache the result
+        if (pair.second) {
+            // Update the entity with all matches in the patterns
+            for (const auto& patEnt : m_mapPatterns) {
+                if (VString::wildmatch(name, patEnt.first)) {
+                    if (!entryr) entryr.reset(new T{});
+                    entryr->update(patEnt.second);
                 }
-                newp->update(wildent.second);
             }
         }
-        return newp;
-    }
-    // Flush on update
-    void flush() VL_MT_SAFE_EXCLUDES(m_mutex) {
-        V3LockGuard lock{m_mutex};
-        m_mapResolved.clear();
+        return entryr.get();
     }
 };
 
@@ -509,7 +506,6 @@ void V3Config::addIgnore(V3ErrorCode code, bool on, const string& filename, int
     } else {
         V3ConfigResolver::s().files().at(filename).addIgnore(code, min, on);
         if (max) V3ConfigResolver::s().files().at(filename).addIgnore(code, max, !on);
-        V3ConfigResolver::s().files().flush();
     }
 }