Skip to content

Support zero-copy intra-process publishing#306

Merged
ahcorde merged 15 commits intoros-perception:rollingfrom
bjsowa:raw_intra_process_pub
May 16, 2024
Merged

Support zero-copy intra-process publishing#306
ahcorde merged 15 commits intoros-perception:rollingfrom
bjsowa:raw_intra_process_pub

Conversation

@bjsowa
Copy link
Copy Markdown
Contributor

@bjsowa bjsowa commented Apr 22, 2024

ROS2 supports intra-process communication when composing multiple nodes in a single process. It additionally can avoid doing any copies of the data when published using std::unique_ptr<MessageT> and subscribed using shared_ptr<const MessageT> or std::unique_ptr<MessageT> (at most 1 subscriber)

This PR is my attempt to add support for publishing using std::unique_ptr without breaking any of the existing APIs.

The idea is that:

  1. Each publisher plugin can decide whether they support publishing using unique ptr (for now I don't see how any other plugin than raw can take advantage of the ownership).
  2. The new Publisher::publish(sensor_msgs::msg::Image::UniquePtr message) method first publishes using the const reference to plugins that don't support std::unique_ptr, then moves the ownership of the message to (at most 1) plugin that supports it.
  3. If there is more than one plugin that supports std::unique_ptr and has subscriptions, we pass the ownership to only one such plugin (the first we find), and for the rest we still use const reference, delegating doing any extra copies to plugin implementations.

Related issues/prs:
#212
#215
#216 (kinda)

Copy link
Copy Markdown
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you planning to make some changes in image_transport_plugins too ? using this new API ?

@bjsowa
Copy link
Copy Markdown
Contributor Author

bjsowa commented Apr 22, 2024

are you planning to make some changes in image_transport_plugins too ? using this new API ?

I don't see how the compressed image transports can take advantage of the image ownership as they still have to copy the data, BUT they might avoid copying the resulting compressed data by publishing it using std::unique_ptr so I was planning to make them use the more generic publish(const sensor_msgs::msg::Image & message, const PublisherT & publisher) method to publish unique_ptr instead of const reference.

@bjsowa bjsowa requested a review from ahcorde April 22, 2024 14:03
@bjsowa
Copy link
Copy Markdown
Contributor Author

bjsowa commented Apr 29, 2024

I marked the method as deprecated. Also, I think that we should not add default implementations that are empty or don't make sense. I also can't make these methods abstract, so instead I just throw exceptions. @ahcorde What do you think?

@bjsowa bjsowa requested a review from ahcorde April 29, 2024 02:12
@bjsowa bjsowa force-pushed the raw_intra_process_pub branch from 09f4756 to 9beb134 Compare May 6, 2024 18:56
@ahcorde ahcorde requested a review from mikeferguson May 15, 2024 11:10
Copy link
Copy Markdown
Member

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me

@ahcorde
Copy link
Copy Markdown
Collaborator

ahcorde commented May 15, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit fd51363 into ros-perception:rolling May 16, 2024
@bjsowa
Copy link
Copy Markdown
Contributor Author

bjsowa commented May 16, 2024

@ahcorde Could this be backported to Jazzy?

@ahcorde
Copy link
Copy Markdown
Collaborator

ahcorde commented May 16, 2024

https://github.com/Mergifyio backport jazzy

@mergify
Copy link
Copy Markdown

mergify bot commented May 16, 2024

backport jazzy

✅ Backports have been created

Details

@Muhammad0312
Copy link
Copy Markdown

@ahcorde could this be back ported to humble or is there some caveat to it?

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.

4 participants