Allow loaned messages without data-sharing

[MiguelCompany]

The only requirement on the Fast DDS API in order for loans to be used is that the type is plain.

This PR allows calling the loan APIs when data-sharing is OFF.
This enables performance improvements on intra-process and UDP.

[richiware]

Maybe definition of using DataSharingKind is not already needed like in file publisher.cpp.

[fujitatomoya]

CI: (only rmw_fastrtps)

• Linux
• Linux-aarch64
• macOS
• Windows

[MiguelCompany]

@fujitatomoya Would you mind triggering CI again on this PR?

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@MiguelCompany still unstable, could you check the log?

[fujitatomoya]

@MiguelCompany friendly ping.

[MiguelCompany]

@fujitatomoya I've been investigating the failures on rclcpp test_subscription_topic_statistics, and I've found the issue.

When using loans, method handle_loaned_message is called on the subscription instead of handle_message. See here
But class SubscriptionTopicStatistics only has an override for handle_message here

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

[fujitatomoya]

CC: @Barry-Xu-2018

[fujitatomoya]

@Barry-Xu-2018 do we have any progress on this?

[Barry-Xu-2018]

@fujitatomoya

I don't try it since this PR isn't merged.
#579 depends on this. So you want to check it now.

[fujitatomoya]

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

i think this needs to be addressed with this PR at the same time. otherwise, we cannot have green light on CI.

[Barry-Xu-2018]

@fujitatomoya

I try this PR and confirm message (pod) can be loaned without data-sharing enabled.

About the failures on rclcpp test_subscription_topic_statistics

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

I have a different opinion. @MiguelCompany

For handle_message(), it has codes to get topic statistics (call handle_message() of SubscriptionTopicStatistics).
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/subscription.hpp#L345-L349

But for handle_loaned_message(), nothing is done for statistics even if statistics is enabled. So test failed.
Therefore, we can add codes to get topic statistics, like codes in handle_message().

https://github.com/ros2/rclcpp/blob/d8e1aed520047b5da28d65cbfbdf781358c14fc6/rclcpp/include/rclcpp/subscription.hpp#L361-L371

[MiguelCompany]

@Barry-Xu-2018 I think you are right.

On my first look at the code, I thought that SubscriptionTopicStatistics was a child class of Subscription, but now I see I was wrong.

The point is what you say. Statistics should be handled when handling a loaned message.

As an extra fix, I think we should also mimic the rest of the behavior on handle_message (i.e. ignore rclcpp intraprocess processing): https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/subscription.hpp#L329-L333

[fujitatomoya]

@Barry-Xu-2018 ah, right what we need to do is modify Subscription::handle_loaned_message. could you work on this? i am happy to do review.

[Barry-Xu-2018]

could you work on this? i am happy to do review.

Okay. I make it.

[Barry-Xu-2018]

@fujitatomoya

I created a PR ros2/rclcpp#1927.
Please help to review.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

i thinik repo file is old, need to be rebased.

[fujitatomoya]

CI withhttps://gist.githubusercontent.com/fujitatomoya/f93a8b37c13378d829aaddcc24bf72f2/raw/89f243a68e8aef6d8dc92e306007e99350c00942/ros2.repos

• Linux
• Linux-aarch64
• Windows

[MiguelCompany]

i thinik repo file is old, need to be rebased.

I can rebase the branch if necessary

[MiguelCompany]

@fujitatomoya I rebased this one.

[fujitatomoya]

@MiguelCompany thanks!

CI:

• Linux
• Linux-aarch64
• Windows

[MiguelCompany]

@fujitatomoya I think the failing tests are wrong, as this line should be with std::launch::async instead of std::launch::deferred, as it is done on the test above.

[fujitatomoya]

CI with ros2/rclcpp#1940

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

I will run whole CI for this, since this could affect many packages.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

Note:

after this PR is merged, rmw_fastrtps tries to enable LoanedMessage to borrow the memory from Fast-DDS as long as message type is bounded (PoD) by default. Application is still responsible to use LoanedMessage Class to use middleware memory for zero-copy inter-process transport.

see also, #596 (comment)

[MiguelCompany]

@fujitatomoya Is there anything pending here?

[fujitatomoya]

I just need to rerun the CI, thanks for the ping.

[fujitatomoya]

CI (Full with rmw_fastrtps static):

• Linux
• Linux-aarch64
• Windows