Refactor topic cache

Signed-off-by: Yadunund <[email protected]>
ros2 · Nov 16, 2023 · 7e2f55a · 7e2f55a
1 parent 2cef08f
commit 7e2f55a
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 13 deletions.
diff --git a/rmw_zenoh_cpp/src/detail/graph_cache.cpp b/rmw_zenoh_cpp/src/detail/graph_cache.cpp
@@ -224,7 +224,7 @@ std::optional<std::pair<std::string, GraphNode>> _parse_token(const std::string
         "Received invalid liveliness token");
       return std::nullopt;
     }
-    GraphNode::PubSubData data;
+    GraphNode::TopicData data;
     data.topic = "/" + std::move(parts[5]);
     data.type = std::move(parts[6]);
     data.qos = std::move(parts[7]);
@@ -242,6 +242,14 @@ std::optional<std::pair<std::string, GraphNode>> _parse_token(const std::string
 }
 } // namespace anonymous
 
+///=============================================================================
+GraphCache::TopicStats::TopicStats(std::size_t pub_count, std::size_t sub_count)
+: pub_count_(pub_count),
+  sub_count_(sub_count)
+{
+  // Do nothing.
+}
+
 ///=============================================================================
 void GraphCache::parse_put(const std::string & keyexpr)
 {
@@ -298,10 +306,12 @@ void GraphCache::parse_put(const std::string & keyexpr)
     // Bookkeeping
     // TODO(Yadunund): Be more systematic about generating the key.
     std::string topic_key = node->pubs.at(0).topic + "?" + node->pubs.at(0).type;
-    auto insertion = graph_topics_.insert(std::make_pair(std::move(topic_key), 1));
+    auto insertion = graph_topics_.insert(std::make_pair(std::move(topic_key), nullptr));
     if (!insertion.second) {
       // Such a topic already exists so we just increment its count.
-      ++insertion.first->second;
+      ++insertion.first->second->pub_count_;
+    } else {
+      insertion.first->second = std::make_unique<TopicStats>(1, 0);
     }
     RCUTILS_LOG_WARN_NAMED(
       "rmw_zenoh_cpp", "Added publisher %s to node /%s in graph.",
@@ -375,12 +385,12 @@ void GraphCache::parse_del(const std::string & keyexpr)
           std::string topic_key = node->pubs.at(0).topic + "?" + node->pubs.at(0).type;
           auto topic_it = graph_topics_.find(topic_key);
           if (topic_it != graph_topics_.end()) {
-            if (topic_it->second == 1) {
+            if (topic_it->second->pub_count_ == 1 && topic_it->second->sub_count_ == 0) {
               // The last publisher was removed so we can delete this entry.
               graph_topics_.erase(topic_key);
             } else {
               // Else we just decrement the count.
-              --topic_it->second;
+              --topic_it->second->pub_count_;
             }
           }
           RCUTILS_LOG_WARN_NAMED(

diff --git a/rmw_zenoh_cpp/src/detail/graph_cache.hpp b/rmw_zenoh_cpp/src/detail/graph_cache.hpp
@@ -72,7 +72,7 @@ class PublishToken
 struct GraphNode
 {
 
-  struct PubSubData
+  struct TopicData
   {
     std::string topic;
     std::string type;
@@ -83,8 +83,8 @@ struct GraphNode
   std::string name;
   // TODO(Yadunund): Should enclave be the parent to the namespace key and not within a Node?
   std::string enclave;
-  std::vector<PubSubData> pubs;
-  std::vector<PubSubData> subs;
+  std::vector<TopicData> pubs;
+  std::vector<TopicData> subs;
 };
 using GraphNodePtr = std::shared_ptr<GraphNode>;
 
@@ -132,11 +132,19 @@ class GraphCache final
   */
   // Map namespace to a map of <node_name, GraphNodePtr>.
   std::unordered_map<std::string, std::unordered_map<std::string, GraphNodePtr>> graph_ = {};
-  // Optimize published topic lookups by caching an unordered_map<string, size_t>
-  // where key is topic_name+topic_type and the value the count.
-  // TODO(Yadunund): Use a single map for both published and subscribed topics.
-  // Consider changing value to pair<size_t, size_t> for pub,sub resp.
-  std::unordered_map<std::string, std::size_t> graph_topics_ = {};
+
+  // Optimize topic lookups mapping "topic_name?topic_type" keys to their pub/sub counts.
+  struct TopicStats
+  {
+    std::size_t pub_count_;
+    std::size_t sub_count_;
+
+    // Constructor which initialized counters to 0.
+    TopicStats(std::size_t pub_count, std::size_t sub_count);
+  };
+  using TopicStatsPtr = std::unique_ptr<TopicStats>;
+  std::unordered_map<std::string, TopicStatsPtr> graph_topics_ = {};
+
   mutable std::mutex graph_mutex_;
 };