[trafficserver] branch 9.2.x updated: Add parent_select plugin strategy caching (#8651)

[zw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 9.2.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/9.2.x by this push:
     new f176214  Add parent_select plugin strategy caching (#8651)
f176214 is described below

commit f1762149c3c628c9a6e5b03756fc3de6742b3552
Author: Robert O Butts <ro...@users.noreply.github.com>
AuthorDate: Mon Feb 14 12:58:32 2022 -0700

    Add parent_select plugin strategy caching (#8651)
    
    Without this, large production strategies.yaml files with thousands
    of remaps take hours to load, because each remap independently
    loads and parses the strategies.yaml file.
    
    This makes the plugin reuse the parsed object if multiple remaps
    use the same file. On reload, files are loaded again.
    
    (cherry picked from commit 4e42bce911e44d158628f5503fcee25a14a25466)
---
 .../parent_select/consistenthash_config.cc         | 30 +++++++++++++++++-
 .../parent_select/consistenthash_config.h          |  3 +-
 .../experimental/parent_select/parent_select.cc    | 36 ++++++++++++----------
 3 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/plugins/experimental/parent_select/consistenthash_config.cc b/plugins/experimental/parent_select/consistenthash_config.cc
index 7186ae4..3203536 100644
--- a/plugins/experimental/parent_select/consistenthash_config.cc
+++ b/plugins/experimental/parent_select/consistenthash_config.cc
@@ -20,6 +20,7 @@
 #include "strategy.h"
 #include "consistenthash.h"
 #include "util.h"
+#include "time.h"
 
 #include <cinttypes>
 #include <string>
@@ -44,6 +45,19 @@
 
 #include "consistenthash_config.h"
 
+namespace
+{
+std::mutex strategies_cache_mutex;
+std::map<std::string, strategies_map> strategies_cache;
+} // namespace
+
+void
+clearStrategiesCache(void)
+{
+  std::lock_guard<std::mutex> guard(strategies_cache_mutex);
+  strategies_cache.clear();
+}
+
 void loadConfigFile(const std::string &fileName, std::stringstream &doc, std::unordered_set<std::string> &include_once);
 
 // createStrategy creates and initializes a Consistent Hash strategy from the given YAML node.
@@ -69,7 +83,15 @@ createStrategiesFromFile(const char *file)
 {
   TSDebug(PLUGIN_NAME, "createStrategiesFromFile plugin createStrategiesFromFile file '%s'", file);
 
-  //  return strategies_map(); // debug
+  {
+    std::lock_guard<std::mutex> guard(strategies_cache_mutex);
+    auto it = strategies_cache.find(file);
+    if (it != strategies_cache.end()) {
+      TSDebug(PLUGIN_NAME, "createStrategiesFromFile file '%s' in cache from previous remap, using cache", file);
+      return it->second;
+    }
+  }
+  TSDebug(PLUGIN_NAME, "createStrategiesFromFile file '%s' not in cache, loading file", file);
 
   YAML::Node config;
   YAML::Node strategies;
@@ -133,6 +155,12 @@ createStrategiesFromFile(const char *file)
       TSDebug(PLUGIN_NAME, "createStrategiesFromFile filename %s got strategy %s emplaced.", basename, name.c_str());
     }
     TSDebug(PLUGIN_NAME, "createStrategiesFromFile filename %s returning strategies created.", basename);
+
+    {
+      std::lock_guard<std::mutex> guard(strategies_cache_mutex);
+      strategies_cache[file] = strategiesMap;
+    }
+
     return strategiesMap;
   } catch (std::exception &ex) {
     TSError("[%s] creating strategies from file %s threw '%s'.", PLUGIN_NAME, file, ex.what());
diff --git a/plugins/experimental/parent_select/consistenthash_config.h b/plugins/experimental/parent_select/consistenthash_config.h
index 3d26e83..d8a3fc7 100644
--- a/plugins/experimental/parent_select/consistenthash_config.h
+++ b/plugins/experimental/parent_select/consistenthash_config.h
@@ -24,7 +24,8 @@
 
 class TSNextHopSelectionStrategy;
 
-typedef std::map<std::string, std::unique_ptr<TSNextHopSelectionStrategy>, std::less<>> strategies_map;
+typedef std::map<std::string, std::shared_ptr<TSNextHopSelectionStrategy>, std::less<>> strategies_map;
 
+void clearStrategiesCache(void);
 strategies_map createStrategiesFromFile(const char *file);
 TSNextHopSelectionStrategy *createStrategy(const std::string &name, const YAML::Node &node);
diff --git a/plugins/experimental/parent_select/parent_select.cc b/plugins/experimental/parent_select/parent_select.cc
index 6641cbb..c6d4a1f 100644
--- a/plugins/experimental/parent_select/parent_select.cc
+++ b/plugins/experimental/parent_select/parent_select.cc
@@ -371,18 +371,8 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuff, int errbuff
   TSDebug(PLUGIN_NAME, "'%s' '%s' successfully created strategies in file %s num %d", remap_from, remap_to, config_file_path,
           int(file_strategies.size()));
 
-  std::unique_ptr<TSNextHopSelectionStrategy> new_strategy;
-
-  for (auto &[name, strategy] : file_strategies) {
-    TSDebug(PLUGIN_NAME, "'%s' '%s' TSRemapNewInstance strategy file had strategy named '%s'", remap_from, remap_to, name.c_str());
-    if (strncmp(strategy_name, name.c_str(), strlen(strategy_name)) != 0) {
-      continue;
-    }
-    TSDebug(PLUGIN_NAME, "'%s' '%s' TSRemapNewInstance using '%s'", remap_from, remap_to, name.c_str());
-    new_strategy = std::move(strategy);
-  }
-
-  if (new_strategy.get() == nullptr) {
+  auto new_strategy = file_strategies.find(strategy_name);
+  if (new_strategy == file_strategies.end()) {
     TSDebug(PLUGIN_NAME, "'%s' '%s' TSRemapNewInstance strategy '%s' not found in file '%s'", remap_from, remap_to, strategy_name,
             config_file_path);
     return TS_ERROR;
@@ -390,7 +380,12 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char *errbuff, int errbuff
 
   TSDebug(PLUGIN_NAME, "'%s' '%s' TSRemapNewInstance successfully loaded strategy '%s' from '%s'.", remap_from, remap_to,
           strategy_name, config_file_path);
-  *ih = static_cast<void *>(new_strategy.release());
+
+  // created a raw pointer _to_ a shared_ptr, because ih needs a raw pointer.
+  // The raw pointer in ih will be deleted in TSRemapDeleteInstance,
+  // which will destruct the shared_ptr,
+  // destroying the strategy if this is the last remap rule using it.
+  *ih = static_cast<void *>(new std::shared_ptr<TSNextHopSelectionStrategy>(new_strategy->second));
 
   // Associate our config file with remap.config to be able to initiate reloads
   TSMgmtString result;
@@ -406,7 +401,8 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri)
 {
   TSDebug(PLUGIN_NAME, "TSRemapDoRemap calling");
 
-  auto strategy = static_cast<TSNextHopSelectionStrategy *>(ih);
+  auto strategy_ptr = static_cast<std::shared_ptr<TSNextHopSelectionStrategy> *>(ih);
+  auto strategy     = strategy_ptr->get();
 
   TSDebug(PLUGIN_NAME, "TSRemapDoRemap got strategy '%s'", strategy->name());
 
@@ -461,6 +457,14 @@ extern "C" tsapi void
 TSRemapDeleteInstance(void *ih)
 {
   TSDebug(PLUGIN_NAME, "TSRemapDeleteInstance calling");
-  auto strategy = static_cast<TSNextHopSelectionStrategy *>(ih);
-  delete strategy;
+  auto strategy_ptr = static_cast<std::shared_ptr<TSNextHopSelectionStrategy> *>(ih);
+  delete strategy_ptr;
+  TSDebug(PLUGIN_NAME, "TSRemapDeleteInstance deleted strategy pointer");
+}
+
+void
+TSRemapPreConfigReload(void)
+{
+  TSDebug(PLUGIN_NAME, "TSRemapPreConfigReload clearing strategies cache");
+  clearStrategiesCache();
 }