From 7204e37721173c3fc76be611c930fb7297c4088f Mon Sep 17 00:00:00 2001
From: Oliver Hamlet <oliver@ortham.io>
Date: Sun, 6 Feb 2022 17:13:17 +0000
Subject: [PATCH] Fix PluginMetadata::NameMatches() performance

This reverts 15781c9b6b37adce5460091dc9ef7cea51c0707b in favour of a
more complete fix that breaks ABI stability.
---
 include/loot/metadata/plugin_metadata.h |  2 ++
 src/api/metadata/plugin_metadata.cpp    | 15 +++++++++++----
 src/api/metadata/yaml/plugin_metadata.h | 19 +++++++------------
 src/api/metadata_list.cpp               | 22 +---------------------
 src/api/metadata_list.h                 |  2 --
 5 files changed, 21 insertions(+), 39 deletions(-)

diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h
index 83b01d62..f19e152a 100644
--- a/include/loot/metadata/plugin_metadata.h
+++ b/include/loot/metadata/plugin_metadata.h
@@ -257,6 +257,8 @@ public:
 
 private:
   std::string name_;
+  std::optional<std::regex> nameRegex_;
+
   std::optional<std::string> group_;
   std::vector<File> loadAfter_;
   std::vector<File> requirements_;
diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp
index 0f69d5d1..43925752 100644
--- a/src/api/metadata/plugin_metadata.cpp
+++ b/src/api/metadata/plugin_metadata.cpp
@@ -45,8 +45,13 @@ PluginMetadata::PluginMetadata() {}
 
 PluginMetadata::PluginMetadata(const std::string& n) : name_(n) {
   // If the name passed ends in '.ghost', that should be trimmed.
-  if (boost::iends_with(name_, ".ghost"))
+  if (boost::iends_with(name_, ".ghost")) {
     name_ = name_.substr(0, name_.length() - 6);
+  }
+
+  if (IsRegexPlugin()) {
+    nameRegex_ = std::regex(name_, std::regex::ECMAScript | std::regex::icase);
+  }
 }
 
 void PluginMetadata::MergeMetadata(const PluginMetadata& plugin) {
@@ -206,9 +211,11 @@ bool PluginMetadata::IsRegexPlugin() const {
 
 bool PluginMetadata::NameMatches(const std::string& pluginName) const {
   if (IsRegexPlugin()) {
-    return std::regex_match(
-        pluginName,
-        std::regex(name_, std::regex::ECMAScript | std::regex::icase));
+    if (!nameRegex_.has_value()) {
+      throw std::runtime_error("Regex plugin does not have regex object");
+    }
+
+    return std::regex_match(pluginName, nameRegex_.value());
   }
 
   return CompareFilenames(name_, pluginName) == 0;
diff --git a/src/api/metadata/yaml/plugin_metadata.h b/src/api/metadata/yaml/plugin_metadata.h
index b1773ce6..a8aae8cf 100644
--- a/src/api/metadata/yaml/plugin_metadata.h
+++ b/src/api/metadata/yaml/plugin_metadata.h
@@ -84,18 +84,13 @@ struct convert<loot::PluginMetadata> {
           node.Mark(),
           "bad conversion: 'name' key missing from 'plugin metadata' object");
 
-    rhs = loot::PluginMetadata(node["name"].as<std::string>());
-
-    // Test for valid regex.
-    if (rhs.IsRegexPlugin()) {
-      try {
-        std::regex(rhs.GetName(), std::regex::ECMAScript | std::regex::icase);
-      } catch (std::regex_error& e) {
-        throw RepresentationException(
-            node.Mark(),
-            std::string("bad conversion: invalid regex in 'name' key: ") +
-                e.what());
-      }
+    try {
+      rhs = loot::PluginMetadata(node["name"].as<std::string>());
+    } catch (std::regex_error& e) {
+      throw RepresentationException(
+          node.Mark(),
+          std::string("bad conversion: invalid regex in 'name' key: ") +
+              e.what());
     }
 
     if (node["group"])
diff --git a/src/api/metadata_list.cpp b/src/api/metadata_list.cpp
index 25031343..81843a55 100644
--- a/src/api/metadata_list.cpp
+++ b/src/api/metadata_list.cpp
@@ -280,8 +280,6 @@ void MetadataList::Clear() {
   unevaluatedPlugins_.clear();
   unevaluatedRegexPlugins_.clear();
   unevaluatedMessages_.clear();
-
-  pluginRegexNamesCache.clear();
 }
 
 std::vector<PluginMetadata> MetadataList::Plugins() const {
@@ -336,25 +334,7 @@ std::optional<PluginMetadata> MetadataList::FindPlugin(
 
   // Now we want to also match possibly multiple regex entries.
   auto nameMatches = [&](const PluginMetadata& pluginMetadata) {
-    // This doesn't call PluginMetadata::NameMatches() because that
-    // creates a std::regex object every time it's called, which is
-    // inefficient. NameMatches() can't be easily improved without
-    // breaking binary compatibility, so instead perform similar logic
-    // but cache plugin name regexes in the MetadataList object.
-    auto name = pluginMetadata.GetName();
-
-    if (pluginMetadata.IsRegexPlugin()) {
-      auto it = pluginRegexNamesCache.find(name);
-      if (it == pluginRegexNamesCache.end()) {
-        auto regex =
-            std::regex(name, std::regex::ECMAScript | std::regex::icase);
-        it = pluginRegexNamesCache.emplace(name, regex).first;
-      }
-
-      return std::regex_match(pluginName, it->second);
-    }
-
-    return CompareFilenames(name, pluginName) == 0;
+    return pluginMetadata.NameMatches(pluginName);
   };
   auto regIt = find_if(regexPlugins_.begin(), regexPlugins_.end(), nameMatches);
   while (regIt != regexPlugins_.end()) {
diff --git a/src/api/metadata_list.h b/src/api/metadata_list.h
index 80642269..ae5798bd 100644
--- a/src/api/metadata_list.h
+++ b/src/api/metadata_list.h
@@ -92,8 +92,6 @@ protected:
   std::vector<PluginMetadata> unevaluatedRegexPlugins_;
   std::vector<Message> unevaluatedMessages_;
 
-  mutable std::unordered_map<std::string, std::regex> pluginRegexNamesCache;
-
   void Load(std::istream& istream, const std::filesystem::path& source_path);
 };
 }