[Galactic] Loan messages implementation #547

MiguelCompany · 2021-07-09T08:39:40Z

This is a backport of #523 into galactic without the need of ros2/rosidl_typesupport_fastrtps#67

* Added is_plain_ attribute to base TypeSupport. * Added new methods to base TypeSupport. * Implementation of rmw_borrow_loaned_message. * Implementation of rmw_return_loaned_message_from_publisher. * Enable loan messages on publishers of plain types. * Implementation for taking loaned messages. * Enable loan messages on subscriptions of plain types. Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]>

Signed-off-by: Miguel Company <[email protected]>

hidmic

Overall LGTM. I'd like to hear @cottsay thoughts on backporting this though.

hidmic · 2021-07-12T16:20:33Z

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

@@ -50,6 +56,7 @@ struct CustomSubscriberInfo : public CustomEventInfo
  const void * type_support_impl_{nullptr};
  rmw_gid_t subscription_gid_{};
  const char * typesupport_identifier_{nullptr};
+  std::shared_ptr<rmw_fastrtps_shared_cpp::LoanManager> loan_manager_;


@MiguelCompany I know these bits are in a grey area when it comes to code visibility but this isn't ABI compatible, strictly speaking.

It is, strictly speaking. But rmw_fastrtps_shared_cpp is designed to be only used by rmw_fastrtps_cpp and rmw_fastrtps_dynamic_cpp, which this PR is also modifying. This means ABI is kept to the users of the RMW.

We need a clearer definition of "public" here. Please correct me if I'm mistaken but as I understand it, this will technically break ABI compatibility even if you don't intend for this struct to be part of the formal rmw_fastrtps* public API. By that, I mean that if we publish this, and I apt update ros-rolling-rmw-fastrtps-shared-cpp without updating the consumers of that package, the consumers won't allocate adequate space for this struct but code in rmw_fastrtps_shared_cpp will try to access it anyway.

One can blur the lines of what constitutes "public API" by claiming that not everything exposed in the headers are formally part of the API, but the linker doesn't care what we say. ABI is ABI, and when you change the size of a struct, it is no longer compatible.

I'm going to bring this up at the next ROS 2 meeting so that we can get some solid documentation on the matter and clearly define our goals regarding ABI stability within a rosdistro so that we can come up with some criteria for when we can break it.

This new field is added at the end, so offsets of the old fields are not modified. The new field is only being accessed from new functions of rmw_fastrtps_shared_cpp. This means that consumers which are not updated will not be calling code accessing the new field.

We discussed this at the ROS 2 meeting, and it sounds like stability among arbitrary combinations of package updates isn't part of our ABI stability goal, so my concerns are no longer valid. Thanks for your patience.

hidmic · 2021-07-12T16:22:09Z

rmw_fastrtps_shared_cpp/src/rmw_take.cpp

+  auto item = loan_mgr->items.emplace_back();
+  if (nullptr == item) {
+    RMW_SET_ERROR_MSG("Out of resources for loaned message info");
+    return RMW_RET_ERROR;


@MiguelCompany nit: RMW_RET_BAD_ALLOC?

Done in 1332adb

Signed-off-by: Miguel Company <[email protected]>

MiguelCompany · 2021-07-23T11:18:24Z

@hidmic @cottsay This is ready for review

hidmic · 2021-07-23T17:05:43Z

rmw_fastrtps_cpp/src/type_support_common.cpp

@@ -47,6 +47,7 @@ void TypeSupport::set_members(const message_type_support_callbacks_t * members)

  // Total size is encapsulation size + data size
  m_typeSize = 4 + data_size;
+  m_typeSize = (m_typeSize + 3) & ~3;


@MiguelCompany mind to explain this change?

@hidmic
TL/DR: Account for RTPS sub-message alignment

The DDSI-RTPS spec requires each submessage to be aligned on a 4-byte boundary. This means that, when using PREALLOCATED memory policy, we could receive a serialized payload bigger than m_typeSize, which would thus be discarded. When using the rmw's default PREALLOCATED_WITH_REALLOC memory policy, an unnecessary reallocation could happen.

I added some comments on f40ff2d

I see. How is this not a problem on master? Upstream Fast-DDS differences?

@hidmic You are right! It is a problem on master. I've created #550 with the proper fix.

Signed-off-by: Miguel Company <[email protected]>

hidmic · 2021-07-26T17:09:15Z

Galactic CI up to test_rmw_implementation and test_communication, against both Fast-DDS based RMW implementations:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-07-29T15:54:35Z

Alright, this is good to go. @cottsay ?

cottsay · 2021-07-29T17:36:10Z

Beyond the one ABI break discussed above, this seems like a straightforward feature addition, so 👍 from me. I'd like to hold off on merging until we discuss it at the ROS 2 meeting on Tuesday, however.

cottsay · 2021-08-04T04:06:53Z

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

@@ -50,6 +56,7 @@ struct CustomSubscriberInfo : public CustomEventInfo
  const void * type_support_impl_{nullptr};
  rmw_gid_t subscription_gid_{};
  const char * typesupport_identifier_{nullptr};
+  std::shared_ptr<rmw_fastrtps_shared_cpp::LoanManager> loan_manager_;


We discussed this at the ROS 2 meeting, and it sounds like stability among arbitrary combinations of package updates isn't part of our ABI stability goal, so my concerns are no longer valid. Thanks for your patience.

hidmic · 2021-08-09T14:18:55Z

Alright. Going in! Thanks for double-checking @cottsay.

* Loan messages implementation (ros2#523) * Added is_plain_ attribute to base TypeSupport. * Added new methods to base TypeSupport. * Implementation of rmw_borrow_loaned_message. * Implementation of rmw_return_loaned_message_from_publisher. * Enable loan messages on publishers of plain types. * Implementation for taking loaned messages. * Enable loan messages on subscriptions of plain types. Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]> * Changes to work with galactic type support. Signed-off-by: Miguel Company <[email protected]> * Improve return code. Signed-off-by: Miguel Company <[email protected]> * Prepare type size for alignment requirements. Signed-off-by: Miguel Company <[email protected]> * Add comments on alignment code. Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Michel Hidalgo <[email protected]>

MiguelCompany and others added 2 commits July 9, 2021 08:28

Changes to work with galactic type support.

df7311f

Signed-off-by: Miguel Company <[email protected]>

hidmic reviewed Jul 12, 2021

View reviewed changes

Improve return code.

1332adb

Signed-off-by: Miguel Company <[email protected]>

cottsay self-requested a review July 15, 2021 21:12

MiguelCompany mentioned this pull request Jul 20, 2021

If loan message is enabled, subscriber will send ack to publisher in take function instead of return loan function, data corruption may happen [13672] eProsima/Fast-DDS#2080

Closed

MiguelCompany force-pushed the loan-messages-galactic branch from cbca746 to f92e78f Compare July 23, 2021 10:57

Prepare type size for alignment requirements.

c61c214

Signed-off-by: Miguel Company <[email protected]>

MiguelCompany force-pushed the loan-messages-galactic branch from f92e78f to c61c214 Compare July 23, 2021 11:05

hidmic reviewed Jul 23, 2021

View reviewed changes

Add comments on alignment code.

f40ff2d

Signed-off-by: Miguel Company <[email protected]>

cottsay approved these changes Aug 4, 2021

View reviewed changes

hidmic merged commit 723a489 into ros2:galactic Aug 9, 2021

MiguelCompany mentioned this pull request Oct 4, 2021

Maximum number of entities per node #563

Closed

MiguelCompany deleted the loan-messages-galactic branch March 7, 2023 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Galactic] Loan messages implementation #547

[Galactic] Loan messages implementation #547

MiguelCompany commented Jul 9, 2021

hidmic left a comment •

edited

Loading

hidmic Jul 12, 2021

MiguelCompany Jul 13, 2021

cottsay Jul 29, 2021

MiguelCompany Jul 30, 2021

cottsay Aug 4, 2021

hidmic Jul 12, 2021

MiguelCompany Jul 13, 2021

MiguelCompany commented Jul 23, 2021

hidmic Jul 23, 2021

MiguelCompany Jul 26, 2021

MiguelCompany Jul 26, 2021

hidmic Jul 26, 2021

MiguelCompany Jul 27, 2021

hidmic commented Jul 26, 2021 •

edited

Loading

hidmic commented Jul 29, 2021

cottsay commented Jul 29, 2021

cottsay Aug 4, 2021

hidmic commented Aug 9, 2021

[Galactic] Loan messages implementation #547

[Galactic] Loan messages implementation #547

Conversation

MiguelCompany commented Jul 9, 2021

hidmic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCompany commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Jul 26, 2021 • edited Loading

hidmic commented Jul 29, 2021

cottsay commented Jul 29, 2021

Choose a reason for hiding this comment

hidmic commented Aug 9, 2021

hidmic left a comment •

edited

Loading

hidmic commented Jul 26, 2021 •

edited

Loading