improve lookup time for matches_any_publishers() by fujitatomoya · Pull Request #3084 · ros2/rclcpp

fujitatomoya · 2026-02-24T06:21:44Z

Description

Fixes #3067 (take 2 🎥 )

Is this user-facing behavior change?

Yes

Did you use Generative AI?

Partially with Claude Opus 4.6

Additional Information

see #3067 (comment)

…3077) This reverts commit 1bf4e6a. Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2026-02-24T06:22:55Z

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

+      // implementation_identifier pointer comparison is not used here because
+      // intra-process communication is always within the same process and RMW,
+      // and pointer comparison is fragile across dynamically loaded components.


this is new compared to #3068. i think we do not need to do the comparison against RMW identifiers.

fujitatomoya · 2026-02-24T06:24:23Z

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp


  mutable std::shared_timed_mutex mutex_;
+
+  GidToPublisherInfoMap gid_to_publisher_info_;


This is the critical ABI fix.

The original placement shifted mutex_'s offset within the class, causing template methods (inlined in downstream packages like Nav2) to access the wrong memory address for mutex_. The corrupted mutex writes overwrote the hashmap's internal state, producing the SIGSEGV in _M_find_before_node.

fujitatomoya · 2026-02-24T06:25:18Z

rclcpp/src/rclcpp/intra_process_manager.cpp

+      // Publisher weak_ptr already expired, fall back to linear scan by pub_id.
+      for (auto git = gid_to_publisher_info_.begin(); git != gid_to_publisher_info_.end(); ++git) {
+        if (git->second.pub_id == intra_process_publisher_id) {
+          gid_to_publisher_info_.erase(git);
+          break;
+        }
+      }


added fall back clean-up process just in case.

Copilot

Pull request overview

This PR addresses performance overhead in rclcpp::experimental::IntraProcessManager::matches_any_publishers() (reported in #3067) by replacing the per-call scan over publishers with a constant-time lookup keyed by publisher GID.

Changes:

Add a rmw_gid_t-keyed unordered_map (gid_to_publisher_info_) to enable O(1) publisher-existence checks by GID.
Populate/maintain the GID map in add_publisher() / remove_publisher() and use it in matches_any_publishers().
Update intra-process manager unit test mocks to provide a stable mock GID and get_gid().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
`rclcpp/src/rclcpp/intra_process_manager.cpp`	Inserts/erases a GID→publisher mapping and switches `matches_any_publishers()` to a hash lookup.
`rclcpp/include/rclcpp/experimental/intra_process_manager.hpp`	Adds `rmw_gid_t` hashing/equality helpers and a new `gid_to_publisher_info_` member type.
`rclcpp/test/rclcpp/test_intra_process_manager.cpp`	Extends mock `PublisherBase` with a mock GID and `get_gid()` to support the new lookup path in tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T06:27:15Z

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

+  /// Equality comparison for rmw_gid_t to enable use in unordered_map
+  struct rmw_gid_equal
+  {
+    bool operator()(const rmw_gid_t & lhs, const rmw_gid_t & rhs) const noexcept
+    {
+      // Compare the data bytes only.
+      // implementation_identifier pointer comparison is not used here because
+      // intra-process communication is always within the same process and RMW,
+      // and pointer comparison is fragile across dynamically loaded components.
+      return std::equal(
+        std::begin(lhs.data),
+        std::end(lhs.data),
+        std::begin(rhs.data));
+    }
+  };


rmw_gid_equal uses std::equal, but this header does not include <algorithm>. Relying on transitive includes can break builds depending on include order; add #include <algorithm> in this header to ensure std::equal is declared.

Copilot · 2026-02-24T06:27:15Z

rclcpp/src/rclcpp/intra_process_manager.cpp

 bool
 IntraProcessManager::matches_any_publishers(const rmw_gid_t * id) const
 {
  std::shared_lock<std::shared_timed_mutex> lock(mutex_);

-  for (auto & publisher_pair : publishers_) {
-    auto publisher = publisher_pair.second.lock();
-    if (!publisher) {
-      continue;
-    }
-    if (*publisher.get() == id) {
-      return true;
-    }
+  // Single O(1) hash map lookup - struct contains both ID and weak_ptr
+  auto it = gid_to_publisher_info_.find(*id);
+  if (it == gid_to_publisher_info_.end()) {
+    return false;


matches_any_publishers() dereferences id unconditionally (find(*id)), which will segfault if a caller passes nullptr. Since the API takes a pointer, consider handling nullptr explicitly (e.g., return false or throw std::invalid_argument) before dereferencing.

Copilot · 2026-02-24T06:27:15Z

rclcpp/src/rclcpp/intra_process_manager.cpp

+          break;
+        }
+      }
+    }


remove_publisher() only erases gid_to_publisher_info_ when the publisher id exists in publishers_. If remove_publisher() is called with an id that is missing/was already erased, the GID map entry will never be removed, leading to stale entries and unbounded growth. Consider always scanning/erasing by pub_id as a fallback even when pub_it == publishers_.end().

Suggested change

}

}

} else {

// Publisher id not found in publishers_, but there might still be a stale

// entry in gid_to_publisher_info_. Fall back to linear scan by pub_id.

for (auto git = gid_to_publisher_info_.begin(); git != gid_to_publisher_info_.end(); ++git) {

if (git->second.pub_id == intra_process_publisher_id) {

gid_to_publisher_info_.erase(git);

break;

}

}

Copilot · 2026-02-24T06:27:16Z

rclcpp/src/rclcpp/intra_process_manager.cpp

+  // Remove GID to publisher info mapping.
+  // First try via the publisher's own GID (fast path).
+  auto pub_it = publishers_.find(intra_process_publisher_id);
+  if (pub_it != publishers_.end()) {
+    auto publisher = pub_it->second.lock();
+    if (publisher) {
+      gid_to_publisher_info_.erase(publisher->get_gid());
+    } else {
+      // Publisher weak_ptr already expired, fall back to linear scan by pub_id.
+      for (auto git = gid_to_publisher_info_.begin(); git != gid_to_publisher_info_.end(); ++git) {
+        if (git->second.pub_id == intra_process_publisher_id) {
+          gid_to_publisher_info_.erase(git);
+          break;


The “fast path” in remove_publisher() (pub_it->second.lock() then publisher->get_gid()) is unlikely to succeed when called from PublisherBase::~PublisherBase() because the weak_ptr lock typically fails once the last shared_ptr is gone. That means removals will usually take the O(N) scan anyway. To keep removals O(1) and simplify control flow, consider storing a pub_id→gid (or pub_id→iterator) mapping at insertion time and erasing by pub_id directly.

Copilot · 2026-02-24T06:27:16Z

rclcpp/test/rclcpp/test_intra_process_manager.cpp

+    auto ptr_value = reinterpret_cast<std::uintptr_t>(this);
+    for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {


The mock GID initialization shifts ptr_value by (i * 8) for all i < RMW_GID_STORAGE_SIZE. When RMW_GID_STORAGE_SIZE * 8 exceeds the bit width of std::uintptr_t (common), shifting by >= width is undefined behavior. Consider copying bytes from ptr_value only up to sizeof(std::uintptr_t) and zero-filling the remaining GID bytes (or memcpy into gid_.data).

Suggested change

auto ptr_value = reinterpret_cast<std::uintptr_t>(this);

for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {

auto ptr_value = reinterpret_cast<std::uintptr_t>(this);

// Zero-fill the GID data to ensure deterministic contents

for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {

gid_.data[i] = 0u;

}

// Copy at most sizeof(std::uintptr_t) bytes from the pointer value to avoid

// shifting by an amount greater than or equal to the bit width of uintptr_t.

const size_t ptr_num_bytes = sizeof(std::uintptr_t);

const size_t bytes_to_copy =

(RMW_GID_STORAGE_SIZE < ptr_num_bytes) ? RMW_GID_STORAGE_SIZE : ptr_num_bytes;

for (size_t i = 0; i < bytes_to_copy; ++i) {

Copilot · 2026-02-24T06:27:16Z

rclcpp/src/rclcpp/intra_process_manager.cpp

 bool
 IntraProcessManager::matches_any_publishers(const rmw_gid_t * id) const
 {
  std::shared_lock<std::shared_timed_mutex> lock(mutex_);

-  for (auto & publisher_pair : publishers_) {
-    auto publisher = publisher_pair.second.lock();
-    if (!publisher) {
-      continue;
-    }
-    if (*publisher.get() == id) {
-      return true;
-    }
+  // Single O(1) hash map lookup - struct contains both ID and weak_ptr
+  auto it = gid_to_publisher_info_.find(*id);
+  if (it == gid_to_publisher_info_.end()) {
+    return false;
  }
-  return false;
+
+  // Verify the publisher still exists by checking the weak_ptr
+  auto publisher = it->second.publisher.lock();
+  return publisher != nullptr;
 }


This PR changes matches_any_publishers() from a linear scan to a hash-map lookup and introduces gid_to_publisher_info_ maintenance. There’s no unit test asserting that matches_any_publishers() returns true after add_publisher() and false after remove_publisher() (and that stale entries don’t cause incorrect results). Adding a focused test would help prevent regressions in the new lookup/removal logic.

mini-1235

Thanks @fujitatomoya! I don't have time right now to run a full performance test again, but based on CPU usage observations, I can clearly see improved performance with this PR!

mini-1235 · 2026-02-24T10:47:29Z

FYI @SteveMacenski

fujitatomoya added 2 commits February 24, 2026 14:54

Reapply "improve lookup time for matches_any_publishers(). (#3068)" (#…

3ff6029

…3077) This reverts commit 1bf4e6a. Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

a few extra fixes to harden the hash map lookup.

90c1e1f

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya requested a review from Copilot February 24, 2026 06:21

fujitatomoya self-assigned this Feb 24, 2026

Copilot started reviewing on behalf of fujitatomoya February 24, 2026 06:22 View session

fujitatomoya commented Feb 24, 2026

View reviewed changes

fujitatomoya mentioned this pull request Feb 24, 2026

Investigate performance of matches_any_publishers() when enabling intraprocess #3067

Open

Copilot AI reviewed Feb 24, 2026

View reviewed changes

mini-1235 approved these changes Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

improve lookup time for matches_any_publishers()#3084

improve lookup time for matches_any_publishers()#3084
fujitatomoya wants to merge 2 commits intorollingfrom
issues/3067-2

fujitatomoya commented Feb 24, 2026

Uh oh!

fujitatomoya Feb 24, 2026

Uh oh!

fujitatomoya Feb 24, 2026

Uh oh!

fujitatomoya Feb 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

mini-1235 left a comment

Uh oh!

mini-1235 commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		mutable std::shared_timed_mutex mutex_;

		GidToPublisherInfoMap gid_to_publisher_info_;

		auto ptr_value = reinterpret_cast<std::uintptr_t>(this);
		for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {

-    auto ptr_value = reinterpret_cast<std::uintptr_t>(this);
-    for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {
+    auto ptr_value = reinterpret_cast<std::uintptr_t>(this);
+    // Zero-fill the GID data to ensure deterministic contents
+    for (size_t i = 0; i < RMW_GID_STORAGE_SIZE; ++i) {
+      gid_.data[i] = 0u;
+    }
+    // Copy at most sizeof(std::uintptr_t) bytes from the pointer value to avoid
+    // shifting by an amount greater than or equal to the bit width of uintptr_t.
+    const size_t ptr_num_bytes = sizeof(std::uintptr_t);
+    const size_t bytes_to_copy =
+      (RMW_GID_STORAGE_SIZE < ptr_num_bytes) ? RMW_GID_STORAGE_SIZE : ptr_num_bytes;
+    for (size_t i = 0; i < bytes_to_copy; ++i) {

Comments

Conversation

fujitatomoya commented Feb 24, 2026

Description

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

Uh oh!

fujitatomoya Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

fujitatomoya Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

fujitatomoya Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

mini-1235 left a comment

Choose a reason for hiding this comment

Uh oh!

mini-1235 commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants