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

Additional improvements to TypeAdapted intra-process comms #1860

Open
clalancette opened this issue Jan 10, 2022 · 1 comment
Open

Additional improvements to TypeAdapted intra-process comms #1860

clalancette opened this issue Jan 10, 2022 · 1 comment
Labels

Comments

@clalancette
Copy link
Contributor

When we merged in #1849 , we did it knowing that there were still improvements that could be made to the code. In particular, there are 2 outstanding issues with that code that we know about:

  1. There is a TODO in the code at
    if (inter_process_publish_needed) {
    ROSMessageType ros_msg;
    // TODO(clalancette): This is unnecessarily doing an additional conversion
    // that may have already been done in do_intra_process_publish_and_return_shared().
    // We should just reuse that effort.
    rclcpp::TypeAdapter<MessageT>::convert_to_ros_message(*msg, ros_msg);
    this->do_intra_process_publish(std::move(msg));
    this->do_inter_process_publish(ros_msg);
    } else {
    this->do_intra_process_publish(std::move(msg));
    }
    . In particular, if we are calling do_intra_process_publish, then it may be the case that as part of that call we are doing a conversion from PublishType -> ROSMessageType so that we can store the ROSMessageType in one or more of the subscribers. But if we did that in do_intra_process_publish, we are unnecessarily doing it again in the publish call directly. The answer is probably to make a new overload of do_intra_process_publish_and_return_shared which always returns the ROSMessageType, and then use that directly when doing the inter-process publish. This probably requires plumbing all the way down to the IntraProcessManager layer.
  2. There's a discussion in Add non transform capabilities for intra-process #1849 (comment) about simplifying the number of cases we need to handle in AnySubscriptionCallback. Currently we allow the signature of the Subscription callback to be a different (though convertible) type from the template type we passed to create_subscription. However, if we force the two of them to be the same, then we can get rid of several case statements when dispatching data. That would help a lot in maintenance since there are a lot of cases handled during dispatch currently.
@clalancette clalancette changed the title Improvements to https://github.com/ros2/rclcpp/pull/1849 Additional improvements to TypeAdapted intra-process comms Jan 27, 2022
@ZhenshengLee
Copy link

@clalancette moving here.

Yes, exactly. If you use intra-process, it can avoid the copy (and the conversion, for that matter) completely, but only if there is a single subscriber.

I will read the code in the PR, but I want to make it clearer for the typical image cuda processing occasion,
Please comfirm if you agree I am right about the following statements.

A: 1 intra-sub-type_adaption

  • no conversion_to_ros
  • the buffer element is smart_ptr of custom_type
  • no copy of custom_type

B: multi intra-sub-type_adapter

C: 1 intra-sub-type_adapter and 1 inter-type_adapter

  • for intra-process, the conversion and copy will be both like occasion A
  • for inter-process, the conversion will happen, the copy of ros_msg will be like normal transimission of ros2, such as (de)serilization, udp/shm transport of dds...
  • the inter-process chain will not affect the behavior of intra-process.
  • the inter-process chain will AFFECT the performance of intra-process because the conversion happens before intra-publish-function
 if (inter_process_publish_needed) { 
   ROSMessageType ros_msg; 
   // in cuda occasion, this conversion may cost much time!
   rclcpp::TypeAdapter<MessageT>::convert_to_ros_message(*msg, ros_msg); 
   this->do_intra_process_publish(std::move(msg)); 
   this->do_inter_process_publish(ros_msg); 
 } else { 

D: 1 intra-sub-type_adapter and 1 inter-sub-rosmsg

this occasion will definitily be the same as occasion C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants