Skip to content

Iterate over IntraProcessBuffer instead of creating a vector - #3272

Closed
thomasmoore-torc wants to merge 2 commits into
ros2:rollingfrom
thomasmoore-torc:intra-process-reduce-copies
Closed

thomasmoore-torc wants to merge 2 commits into
ros2:rollingfrom
thomasmoore-torc:intra-process-reduce-copies

Conversation

@thomasmoore-torc

Copy link
Copy Markdown

rclcpp: Reduce unnecessary copies/allocations in intra-process publish

Branch: intra-process-reduce-copies (this repo: ros2/rclcpp)
Related PR: #3271 — "Populate message info for intra-process messages" (intra-process-message-info). See Landing order below — no functional dependency, but the two touch overlapping code.

Summary

Two avoidable allocation/copy hot spots in the intra-process publish path:

  1. IntraProcessManager recomputes and concatenates the take-ownership subscription ID lists (inter-process-need + intra-process-need) into a fresh std::vector on every single publish call, even though the set of registered subscriptions only changes on add_subscription/remove_subscription.
  2. Code that needs to visit every message currently buffered for a subscription (e.g. transient-local replay to a newly-added subscription) builds a full std::vector copy of the ring buffer's contents first, then iterates it, rather than visiting the buffer's storage directly.

Changes

  • Pre-compute concatenated take ownership subscriptions — caches the merged take-ownership subscription id list, invalidating/recomputing it only when subscriptions are added or removed, instead of rebuilding it on every publish.
  • Iterate over IntraProcessBuffer instead of creating a vector — adds for_each_shared/for_each_unique visitor-style iteration over IntraProcessBuffer's ring-buffer storage (covering all shared/unique storage × shared/unique request combinations, including the move-only unique-ptr case), and switches vector-materializing call sites to use it instead.

Compatibility

Internal to rclcpp's intra-process implementation (rclcpp::experimental::*); no public API or observable behavior changes — this is a pure performance/allocation-reduction change.

Testing

Built and tested against rolling in a ros:rolling-ros-base container via colcon build/colcon test for rclcpp (3091 tests, 0 failures), plus uncrustify/cpplint lint targets. All green.

Landing order

intra-process-message-info (opened separately) touches the same files (intra_process_buffer.hpp, intra_process_manager.hpp, and both intra-process test files) as part of an unrelated feature adding message-info propagation. The two branches are independently buildable and testable, but not free to merge in either order without a rebase — real textual conflicts exist between them (confirmed via a trial merge). No functional dependency either way; whichever of the two merges second will need to reconcile against the other.

🤖 Generated with Claude Code

(cherry picked from commit 1b6ee8a5a4fbc447c97c55007b7eb2049cb1be47)
Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
(cherry picked from commit 13aae0cbbdb0910213318305c0df176eefe248d6)
Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
@jmachowinski

Copy link
Copy Markdown
Collaborator

This AI generated PR description is pretty muss useless.
What is the goal here?
Did you somehow verify that there is a real performance enhancement ?

@thomasmoore-torc

Copy link
Copy Markdown
Author

The goal is to eliminate unnecessary memory allocations. This is achieved by iterating over the IntraPreocessBuffer instead of having it create and return a std::vector.

@jmachowinski

Copy link
Copy Markdown
Collaborator

Did you somehow verify that there is a real performance enhancement ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants