improve lookup time for matches_any_publishers()#3084
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
There was a problem hiding this comment.
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-keyedunordered_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 inmatches_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.
mini-1235
left a comment
There was a problem hiding this comment.
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!
|
FYI @SteveMacenski |
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
|
@mini-1235 do we have regression with this PR like #3068 (comment)? (that is what i am really interested in before merge... 😅 ) |
|
Pulls: #3084
|
No, my comment above reflects the results from testing in Nav2, and I did not observe any regressions this time |
|
Adds O(1) hash-map lookup for matches_any_publishers() instead of linear scan over all publishers. Test changes excluded (jazzy mock class differs from rolling). Upstream: ros2#3084 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds O(1) hash-map lookup for matches_any_publishers() instead of linear scan over all publishers. Test changes excluded (jazzy mock class differs from rolling). Upstream: ros2#3084 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)