Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve intraprocess communication behavior #649

Closed
3 tasks done
mjcarroll opened this issue Jan 22, 2019 · 13 comments
Closed
3 tasks done

Improve intraprocess communication behavior #649

mjcarroll opened this issue Jan 22, 2019 · 13 comments
Assignees
Labels
enhancement
Milestone

Comments

@mjcarroll
Copy link
Member

@mjcarroll mjcarroll commented Jan 22, 2019

There have been some performance issues uncovered in the behavior and assumptions made in the implementation of intraprocess communications. This is a meta-ticket to track improvements to intraprocess communications planned for D-turtle.

Tasks:

  • Document talk between @mjcarroll and @wjwwood that describes current behavior and enumerates properties of various publisher/subscription types.
  • Use rmw_count_matched_* to avoid publishing interprocess when a publication only has intraprocess subscriptions.
    • In the current system, a copy of the message must be made in order for both interprocess and intraprocess communications to have a copy that is owned by the respective system. Given the newly exposed subscription/publication count, this copy could be avoided.
    • This should be doable by checking the total subscription count vs the intraprocess subscription count.
    • Done in ros2/rclcpp#628
  • Store unique or shared message in intraprocess manager based on publisher/subscription type.
    • In the current system, it is assumed that unique_ptr is used on both the publisher and subscription side. If the intraprocess manager was capable of storing shared_ptr, and subscriptions received shared_ptr (read only), then copies could be avoided in the case of multiple intraprocess subscriptions as well as mixed intra/interprocess subscriptions
    • Done in: ros2/rclcpp#690
  • (stretch) offer "direct dispatch" (C++ only) - This would be similar to how ROS1 nodelets worked in that subscriptions could be directly invoked from the publication-side publish method. This is particularly useful in processing pipeline environments

Based on:

References:

@mjcarroll mjcarroll added the enhancement label Jan 22, 2019
@mjcarroll mjcarroll added this to the d-turtle milestone Jan 22, 2019
@mjcarroll mjcarroll self-assigned this Jan 22, 2019
@dirk-thomas dirk-thomas mentioned this issue Jan 22, 2019
20 tasks
@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Jan 22, 2019

Use rmw_count_matched_* to avoid publishing interprocess when a publication only has intraprocess subscriptions.

I am not sure this is feasible. The publisher could use QoS settings to deliver previously published messages to late joining subscribers. In that case skipping the inter process publish just because no subscribers are present at the moment would be wrong.

@wjwwood
Copy link
Member

@wjwwood wjwwood commented Jan 22, 2019

I am not sure this is feasible. The publisher could use QoS settings to deliver previously published messages to late joining subscribers. In that case skipping the inter process publish just because no subscribers are present at the moment would be wrong.

Obviously for non volatile durability you have to publish anyways, but yes it's worth noting that this only works if there are no subscriptions and durability is volatile.

@cottsay cottsay added the ready label Jan 24, 2019
@hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 30, 2019

@mjcarroll did put down your offline discussion w/ @wjwwood in a document? Maybe I can start pushing one of these bullets. Which one's the smallest? :)

@mjcarroll
Copy link
Member Author

@mjcarroll mjcarroll commented Feb 12, 2019

Probably not ready for a PR, but includes more notes from our discussion: https://github.com/ros2/design/blob/mjcarroll/intraprocess/articles/160_intraprocess_communications.md

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Apr 18, 2019

Latest PR on this topic: ros2/rclcpp#690.

@alsora
Copy link

@alsora alsora commented May 1, 2019

Hi, I was having a look at the current intra process manager.
Are there plans to remove the do_intra_process_publish function that still relies on the DDS?
https://github.com/ros2/rclcpp/blob/98f610c114b8cc7bda2ba3a3d1d486767ee293e5/rclcpp/include/rclcpp/publisher.hpp#L91

If no, could I ask which are the reasons why it's required?

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented May 1, 2019

Are there plans to remove the do_intra_process_publish function that still relies on the DDS?

That function is performing the intra process message passing functionality which is a rclcpp concept and has nothing to do with DDS? So no, since that function provide the zero-copy reference passing of messages it is there for a reason.

@alsora
Copy link

@alsora alsora commented May 1, 2019

@dirk-thomas From what I'm understanding, do_intra_process_publish, calls rcl_publish in order to publish a message of type IntraProcessMessage. This goes through rmw_publish and so through the DDS as usual.

Could you explain me why you say that it's not involving the DDS?

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented May 1, 2019

Well, it does publish a meta-message but it uses the RMW interface for this. So you could use a non-DDS RMW implementation.

Can you clarify what you are asking for? If there are any plans to make the intra process communication without sending any meta-message through the RMW impl.?

@alsora
Copy link

@alsora alsora commented May 1, 2019

Yes, exactly.
My idea would be to use a ROS2 system with a standard DDS (i.e. Fast-RTPS).
In this system, if publisher and subscriber use both IPC I don't see why I need a meta-message, I could just trigger the subscriber callback from the IPC manager or have some buffers for each subscriber and push the pointer there.

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented May 1, 2019

While that would be an option there are also downsides to that approach. When you switch from inter- to intra- your behavior might change if the intra- part doesn't implement the very exact behavior than the inter- in terms of queuing, QoS, etc. You find some information about the topic in the ROSCon 2015 slides (I couldn't find anything better).

Currently there is no plan to change the implementation but there is always room for improvements.

@wjwwood
Copy link
Member

@wjwwood wjwwood commented May 6, 2019

What @dirk-thomas said is right, basically sending the "envelope" message via rmw (DDS or whatever it may use) allows us to avoid a lot of work needed to emulate QoS features like deadline, lifespan, keep last vs keep all history, etc... Also, it prevents us from having to emulate scheduling (publish should be async, queuing the data but not calling the subscription directly).

We could implement our own queuing to replace that, but it would be a good deal of work if we also wanted to have consistent behavior, which we do. That being said, we may still do that in the future, but for now this gives a significant improvement for large data, so it's an incremental improvement.

There's also this idea of "direct dispatch" which is what ROS 1 did, where you have publish directly call subscription callbacks, one after another. The problem with this is that if you add a subscription callback that takes a long time, now your publish function will suddenly take substantially longer. As a default behavior we're trying to avoid things like that. However, some people may desire that behavior, so we think we'd like to try to provide that as an option in the future along side the default which keeps publish async.

@mjcarroll
Copy link
Member Author

@mjcarroll mjcarroll commented May 16, 2019

Since the three main tasks have been addressed, this issue can be closed.

@mjcarroll mjcarroll removed the ready label May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

No branches or pull requests

6 participants