Add non transform capabilities for intra-process

[gonzodepedro]

Signed-off-by: Gonzalo de Pedro [email protected]

[clalancette]

For context, the point of this PR is to enhance TypeAdaptation so that it avoids unnecessary conversions. In particular, if a Publisher and Subscription are both in the same context, using intra-process communications, and use the same custom TypeAdapted type, then we can just place the published data directly from the publisher into the Subscription queue for delivery without doing any conversions. If any of several other situations happen (Publisher is ROS message, Subscription is custom, Publisher is custom, Subscription is ROS message, etc), then we do the conversion as necessary.

This has two primary benefits, both related to performance:

1. We can avoid doing the (possibly) expensive and unnecessary to a ROS type on the publisher, and then from the ROS type on the subscription.
2. We can use a custom type to hold onto custom "handles", allowing the Publisher to pass those handles directly to the Subscription. Those handles might be handles to hardware accelerators, which means that the system can use the hardware accelerator to do the conversion without synchronizing with the CPU to publish to the next node.

[clalancette]

Here's preliminary CI to see where we stand:

• Linux
• Linux-aarch64
• macOS
• Windows

[alsora]

I'm not sure I understand the logic with the hierarchy in the Subscription IPC class.
We have that:

SubscriptionIntraProcess is a
SubscriptionIntraProcessBuffer is a
ROSMessageIntraProcessBuffer is a
SubscriptionIntraProcessBase is a
Waitable

Do we really need all these layers?

[audrow]

Yes, I agree this is confusing. The most confusing part to me is going from Subscription to ROSMessage back to Subscription. Maybe rename ROSMessageIntraProcessBuffer to something with subscription in it, or fold it in to another class in the inheritance hierarchy?

SubscriptionIntraProcessBuffer is a
ROSMessageIntraProcessBuffer is a
SubscriptionIntraProcessBase is a

[clalancette]

Do we really need all these layers?

I agree that this is confusing, but I think we do need all of these layers.

So at the base, the whole thing needs to be a Waitable because it needs to be waited on by the executors. We need the SubscriptionIntraProcessBase as a non-templated type so that we can store essentially untyped subscriptions in the IntraProcessManager (

rclcpp/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

Lines 319 to 320 in c8c4851

	using SubscriptionMap =
	std::unordered_map<uint64_t, rclcpp::experimental::SubscriptionIntraProcessBase::WeakPtr>;

). The split between ROSMessageIntraProcessBuffer and SubscriptionIntraProcessBuffer is how the subscriber ultimately communicates whether it is a ROS type or not to the IntraProcessManager. So we need both of those layers to distinguish between those two cases. And finally, we need SubscriptionIntraProcess as the thing that the Subscription class interacts with.

I'll note that this hierarchy was largely here already; the only thing this PR does is to add in the ROSMessageIntraProcessBuffer.

One thing we might be able to do is collapse the SubscriptionIntraProcess and SubscriptionIntraProcessBuffer classes into one. Neither of them does too much on its own, and it isn't entirely clear why they are separated. If you'd like me to try to do that, I'll give that a whirl. I think we need the rest of the layers.

[audrow]

I'll note that this hierarchy was largely here already; the only thing this PR does is to add in the ROSMessageIntraProcessBuffer.

Do you think that it makes sense to rename ROSMessageIntraProcessBuffer to something like SubscriptionROSMessageIntraProcessBuffer, or something similar? It is just a little odd to me that it's in the hierarchy but doesn't start with Subscription, like the other classes after waitable.

If you'd like me to try to do that, I'll give that a whirl. I think we need the rest of the layers.

Maybe make an issue and possibly do it in another PR? This isn't that high priority for me.

[clalancette]

Do you think that it makes sense to rename ROSMessageIntraProcessBuffer to something like SubscriptionROSMessageIntraProcessBuffer, or something similar? It is just a little odd to me that it's in the hierarchy but doesn't start with Subscription, like the other classes after waitable.

That makes sense to me. I'll go ahead and rename it (probably to SubscriptionROSMsgIntraProcessBuffer, just to shorten it a bit).

[alsora]

I just gave a quick look at this from a style point of view.
I need to take a little bit more time to comment on the actual implementation, but my first impression seems good to me.

However, I want to point out that I'm a little bit worried about the increase in complexity caused by adding these new capabilities.
I wonder how will we implement the next feature (e.g. intra-process support for loaned messages), i.e. if we will keep having to exponentially grow all those if-else conditions in various parts of the codebase.

[clalancette]

(force-pushed to resolve DCO)

[audrow]

A few nitpicks but the code and tests look good to me. LGTM with green CI for all packages above rclcpp.

[audrow]

Yes, I agree this is confusing. The most confusing part to me is going from Subscription to ROSMessage back to Subscription. Maybe rename ROSMessageIntraProcessBuffer to something with subscription in it, or fold it in to another class in the inheritance hierarchy?

SubscriptionIntraProcessBuffer is a
ROSMessageIntraProcessBuffer is a
SubscriptionIntraProcessBase is a

[clalancette]

All right, with the latest fixes on macOS, let's see what CI here looks like:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

this case cannot happen, right?

Now for intraprocess communication, the intraprocess buffer always stores instances of SubscribedType, and that will match the type specified in the callback.
We don't need a special code path for this case AFAIS.

I also don't think the possible types should have two specializations for ROS messages and type adapters, the possible callbacks are the same in both cases and the argument is always a kind of reference/pointer to SubscribedType.

In the only case we need to aware of the "ROS message type" is in interprocess dispatch.

[gonzodepedro]

Even if the buffer is of SubscribedType, the callback may require a ROSMessage.

[ivanpauno]

Even if the buffer is of SubscribedType, the callback may require a ROSMessage.

How?
We're creating a buffer type based on the callback type.

i.e. if your callback is a smart_ptr<ROSMessage> or a ROSMessage & the buffer will also be a smart pointer to a ROSMessage.

Thus, all those code paths seem unnecessary, and we do have a lot of code paths!

[clalancette]

i.e. if your callback is a smart_ptr or a ROSMessage & the buffer will also be a smart pointer to a ROSMessage.

I don't think this is the case (though there is a lot going on here, so I could be wrong).

There can be a difference between the type that the subscription was created with, and the type that the callback is. Consider the following (modified from one of the tests):

// Assume we defined the TypeAdapter already
using StringTypeAdapter = rclcpp::TypeAdapter<std::string, rclcpp::msg::String>;
auto callback =
  [message_data, &is_received](
  const rclcpp::msg::String & msg) -> void
  {
    // do something
  }
auto sub = node->create_subscription<StringTypeAdapter>(topic_name, 1, callback);

When we create the subscription, through many layers we end up creating the buffer as a std::string (since that is the SubscribedType here). But our callback is a ROSMessage type, so we need these conversion routines to do the conversion right before we deliver to the callback.

[ivanpauno]

(edit) Read next comment, this one can be ignored.

---

I don't think that example makes sense, specifying StringTypeAdapter is not really useful if the callback is expecting a msg::String.
i.e. what's the benefit of doing that instead of just using create_subscription<msg::String>?
If there's no benefit, we shouldn't support it.
It adds complexity because you need to keep track of more combinations in the implementation.

I also think we cannot really allow that kind of code if we want the compiler to be able to infer the correct Subscription type from the callback type in Node::create_subscription().

i.e. being able to write

auto sub = node->create_subscription(topic_name, 1, callback);  // subscription type inferred from callback type

I think there's no way to infer it if both a Subscription<msg::String> and a Subscription<StringTypeAdapter> accept a msg::String callback. Though I might be wrong, c++ templates and specialization rules are complex and I cannot really remember them.
I have also not kept track of what's currently possible, I have a few questions:

• is it possible to mix callback and subscription type as the example above?
• is automatic template parameter inference still working for create_subscription()? IIUC, that used to work (*).

If the answer to both those question is yes, my only reason to not support that is that I don't see the benefit and it adds complexity.
I guess we can ignore that, but if we later want to not support that to reduce complexity it will be an API breaking change.

(*) If we currently support code as the example above, I guess it will only break if no type is passed and a type adapter is available, though it will still work if there's no type adapter for that message type.
I can also imagine a long and impossible to understand compiler error, which is also something to check and consider.

[ivanpauno]

is automatic template parameter inference still working for create_subscription()? IIUC, that used to work (*).

Sorry, my memory is bad, I think we never supported this.
My concern about breaking template parameter inference can be ignored.

The only part I think that it's still valid from my last comment is that supporting that example seems unnecessary, but considering that we already do support it, I think we can move forward.

[clalancette]

The only part I think that it's still valid from my last comment is that supporting that example seems unnecessary, but considering that we already do support it, I think we can move forward.

I talked to @audrow and @wjwwood a little bit about this, and part of the idea was so that you could potentially have multiple TypeAdapters for the same types together. Passing the correct TypeAdapter type to the subscription would let the compiler know which of them to choose. That said, I'll let @wjwwood comment here, as he has the best long-term vision for this.

Regardless, I think I'm going to move forward here and address the (minor) points raised by @audrow , and then run CI on the lot again.

[ivanpauno]

Regardless, I think I'm going to move forward here and address the (minor) points raised by @audrow , and then run CI on the lot again.

Moving forward sounds good.

Passing the correct TypeAdapter type to the subscription would let the compiler know which of them to choose

That's fine.
My comment is about the need to support Subscription<TypeAdapterT>(..., CallbackUsingRosMessageT callback, ...), i.e. allowing to pass a callback that takes some smart pointer or referece to a ROS message type while asking for a TypeAdapterT subscription.
In practice, if the user replace TypeAdapterT with RosMessageT the implementation will do behind the scenes the exact same thing.
Allowing that adds a lot of code paths in the dispatching logic, but it doesn't seem to add any value.

Putting it another way, you always want the intraprocess buffer to match the type expected by the callback (as in that way it's possible to avoid the most copies).
I assumed that the type expected by the callback was always the Subscription<...> template parameter type, but that doesn't seem to actually be the case.
The intraprocess buffer type can currently be different to the callback type, as it's chosen basen on the subscription type, which is suboptimal.
As commented in the previous paragrah, there doesn't seem to be a real reason to allow mixing a RosMessageT callback type with a TypeAdapterT subscription.

// this two subscriptions will dispatch intraprocess and interprocess messages in the exact same way
// their type is different for the cpp type system, but that doesn't seem to have any usefulness.
// my proposal is to not support creating sub2, only sub1
// (edit) actually, the second alternative sub2 currently dispatches intraprocess messages more inefficiently.
//   But they should be doing the same thing really, so I think we should support one to reduce complexity.
auto callback = [] (msg::MyMsg::SharedPtr msg) {...};
rclcpp::Subscription<msg::MyMsg> sub1 = node->create_subscription<msg::MyMsg>(..., callback);
rclcpp::Subscription<MyTypeAdaptedToMsg> sub2 = node->create_subscription<MyTypeAdaptedToMsg>(..., callback);

[clalancette]

I've opened up #1860 to track this (and one other issue) for the future.

[audrow]

Looks good to me - nice job with the tests and in refactoring!

[audrow]

I'll note that this hierarchy was largely here already; the only thing this PR does is to add in the ROSMessageIntraProcessBuffer.

Do you think that it makes sense to rename ROSMessageIntraProcessBuffer to something like SubscriptionROSMessageIntraProcessBuffer, or something similar? It is just a little odd to me that it's in the hierarchy but doesn't start with Subscription, like the other classes after waitable.

If you'd like me to try to do that, I'll give that a whirl. I think we need the rest of the layers.

Maybe make an issue and possibly do it in another PR? This isn't that high priority for me.

[clalancette]

CI on the latest:

• Linux
• Linux-aarch64
• macOS
• Windows
• Windows Debug
• RHEL
• Clang

[clalancette]

All right, CI is in really good shape, and I have an approval. I'm going to go ahead and merge this one, if there are any lingering things we want to fix to clean it up and/or reduce complexity, we can do it in a follow-up PR.