From 77db1ed25b4c26ab722a13c268521b923606c04d Mon Sep 17 00:00:00 2001
From: Chris Lalancette <clalancette@gmail.com>
Date: Wed, 23 Aug 2023 08:15:44 -0400
Subject: [PATCH] Revamp list_parameters to be more efficient and easier to
 read. (#2282)

1. Use constref for the loop variable.
2. Do more work outside of the loop.
3. Skip doing unnecessary work where we can inside the loop.

With this in place, I measured about a 7% performance
improvement over the previous implementation.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
---
 .../node_interfaces/node_parameters.cpp       | 69 +++++++++++--------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
index c8bb62e238..b4c899ff9b 100644
--- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
+++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
@@ -1038,37 +1038,50 @@ NodeParameters::list_parameters(const std::vector<std::string> & prefixes, uint6
   // TODO(mikaelarguedas) define parameter separator different from "/" to avoid ambiguity
   // using "." for now
   const char * separator = ".";
-  for (auto & kv : parameters_) {
-    bool get_all = (prefixes.size() == 0) &&
-      ((depth == rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE) ||
-      (static_cast<uint64_t>(std::count(kv.first.begin(), kv.first.end(), *separator)) < depth));
-    bool prefix_matches = std::any_of(
-      prefixes.cbegin(), prefixes.cend(),
-      [&kv, &depth, &separator](const std::string & prefix) {
-        if (kv.first == prefix) {
-          return true;
-        } else if (kv.first.find(prefix + separator) == 0) {
-          size_t length = prefix.length();
-          std::string substr = kv.first.substr(length);
-          // Cast as unsigned integer to avoid warning
-          return (depth == rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE) ||
-          (static_cast<uint64_t>(std::count(substr.begin(), substr.end(), *separator)) < depth);
-        }
-        return false;
-      });
-    if (get_all || prefix_matches) {
-      result.names.push_back(kv.first);
-      size_t last_separator = kv.first.find_last_of(separator);
-      if (std::string::npos != last_separator) {
-        std::string prefix = kv.first.substr(0, last_separator);
-        if (
-          std::find(result.prefixes.cbegin(), result.prefixes.cend(), prefix) ==
-          result.prefixes.cend())
-        {
-          result.prefixes.push_back(prefix);
+
+  auto separators_less_than_depth = [&depth, &separator](const std::string & str) -> bool {
+      return static_cast<uint64_t>(std::count(str.begin(), str.end(), *separator)) < depth;
+    };
+
+  bool recursive = (prefixes.size() == 0) &&
+    (depth == rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE);
+
+  for (const std::pair<const std::string, ParameterInfo> & kv : parameters_) {
+    if (!recursive) {
+      bool get_all = (prefixes.size() == 0) && separators_less_than_depth(kv.first);
+      if (!get_all) {
+        bool prefix_matches = std::any_of(
+          prefixes.cbegin(), prefixes.cend(),
+          [&kv, &depth, &separator, &separators_less_than_depth](const std::string & prefix) {
+            if (kv.first == prefix) {
+              return true;
+            } else if (kv.first.find(prefix + separator) == 0) {
+              if (depth == rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE) {
+                return true;
+              }
+              std::string substr = kv.first.substr(prefix.length());
+              return separators_less_than_depth(substr);
+            }
+            return false;
+          });
+
+        if (!prefix_matches) {
+          continue;
         }
       }
     }
+
+    result.names.push_back(kv.first);
+    size_t last_separator = kv.first.find_last_of(separator);
+    if (std::string::npos != last_separator) {
+      std::string prefix = kv.first.substr(0, last_separator);
+      if (
+        std::find(result.prefixes.cbegin(), result.prefixes.cend(), prefix) ==
+        result.prefixes.cend())
+      {
+        result.prefixes.push_back(prefix);
+      }
+    }
   }
   return result;
 }