Add cache for topic filter to avoid performance burden on checks

Rationale: When rosbag2 discovery enabled we are continuously doing a lot of checks in the TopicFilter::take_topic(..) which could be avoided by using cache Signed-off-by: Michael Orlov <[email protected]>
ros2 · Oct 24, 2023 · 55eacbe · 55eacbe
1 parent 8216ea2
commit 55eacbe
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 12 deletions.
diff --git a/rosbag2_transport/include/rosbag2_transport/topic_filter.hpp b/rosbag2_transport/include/rosbag2_transport/topic_filter.hpp
@@ -62,6 +62,7 @@ class ROSBAG2_TRANSPORT_PUBLIC TopicFilter
   bool allow_unknown_types_ = false;
   std::unordered_set<std::string> already_warned_unknown_types_;
   rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_;
+  std::unordered_map<std::string, bool> take_topics_cache_;
 };
 }  // namespace rosbag2_transport
 

diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp
@@ -75,15 +75,15 @@ topic_is_unpublished(
   const std::string & topic_name, rclcpp::node_interfaces::NodeGraphInterface & node_graph)
 {
   auto publishers_info = node_graph.get_publishers_info_by_topic(topic_name);
-  return publishers_info.size() == 0;
+  return publishers_info.empty();
 }
 
 bool
 is_leaf_topic(
   const std::string & topic_name, rclcpp::node_interfaces::NodeGraphInterface & node_graph)
 {
   auto subscriptions_info = node_graph.get_subscriptions_info_by_topic(topic_name);
-  return subscriptions_info.size() == 0;
+  return subscriptions_info.empty();
 }
 }  // namespace
 
@@ -94,21 +94,31 @@ TopicFilter::TopicFilter(
   RecordOptions record_options,
   rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
   bool allow_unknown_types)
-: record_options_(record_options),
+: record_options_(std::move(record_options)),
   allow_unknown_types_(allow_unknown_types),
   node_graph_(node_graph)
 {}
 
-TopicFilter::~TopicFilter()
-{}
+TopicFilter::~TopicFilter() = default;
 
 std::unordered_map<std::string, std::string> TopicFilter::filter_topics(
   const std::map<std::string, std::vector<std::string>> & topic_names_and_types)
 {
   std::unordered_map<std::string, std::string> filtered_topics;
   for (const auto & [topic_name, topic_types] : topic_names_and_types) {
-    if (take_topic(topic_name, topic_types)) {
-      filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
+    // Check take_topics_cache_ first to avoid performance burden when discovery thread running
+    auto take_topics_cache_it = take_topics_cache_.find(topic_name);
+    if (take_topics_cache_it != take_topics_cache_.end()) {
+      if (take_topics_cache_it->second) {
+        filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
+      }
+    } else {
+      if (take_topic(topic_name, topic_types)) {
+        filtered_topics.insert(std::make_pair(topic_name, topic_types[0]));
+        take_topics_cache_[topic_name] = true;
+      } else {
+        take_topics_cache_[topic_name] = false;
+      }
     }
   }
   return filtered_topics;
@@ -117,6 +127,11 @@ std::unordered_map<std::string, std::string> TopicFilter::filter_topics(
 bool TopicFilter::take_topic(
   const std::string & topic_name, const std::vector<std::string> & topic_types)
 {
+  if (!has_single_type(topic_name, topic_types)) {
+    return false;
+  }
+  const std::string & topic_type = topic_types[0];
+
   if (!record_options_.include_unpublished_topics && node_graph_ &&
     topic_is_unpublished(topic_name, *node_graph_))
   {
@@ -147,18 +162,13 @@ bool TopicFilter::take_topic(
     return false;
   }
 
-  if (!has_single_type(topic_name, topic_types)) {
-    return false;
-  }
-
   if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) {
     RCUTILS_LOG_WARN_ONCE_NAMED(
       ROSBAG2_TRANSPORT_PACKAGE_NAME,
       "Hidden topics are not recorded. Enable them with --include-hidden-topics");
     return false;
   }
 
-  const std::string & topic_type = topic_types[0];
   if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) {
     return false;
   }