Description

No long detailed description this time, unfortunately...

This issue describes and lists the relevant PRs that build on top of the PRs in https://github.com/ros2/ros2/issues/1374 with non-essential, bonus changes for ease of review and decoupling.

The base PRs should be prioritized.

The PRs in #1374 assumed that most of the objects being introduced can only live on the heap, but this wave of PRs allows those objects to be created on the stack and then initialized.

The changes here are namely refactors across the entire stack for function signatures to take in allocators, and splitting of create() functions into init() and create() (likewise for destroy into fini() and destroy().)

That is, splitting the steps of:

Allocating an outer struct (create/new)
Allocating and initializing the internal members of that struct (init)

And similarly for destruction:

Deallocating the outer struct (delete/destroy)
Deallocating and finalizing the internal members of that struct (fini)

NOTE

These changes will be occurring mostly in the rmw and rosidl layers

Relevant PRs

[x] https://github.com/ros2/rcutils/pull/421
[x] https://github.com/ros2/rosidl/pull/737
[x] https://github.com/ros2/rosidl_dynamic_typesupport/pull/2
[x] https://github.com/ros2/rosidl_dynamic_typesupport_fastrtps/pull/3
[x] https://github.com/ros2/rmw/pull/353
[x] ~~https://github.com/ros2/rclcpp/pull/2160~~
- [ ] https://github.com/ros2/rclcpp/pull/2176
[x] https://github.com/ros2/rcl/pull/1057
[x] RMWs (must be merged together)
- [x] https://github.com/ros2/rmw_implementation/pull/219
- [x] https://github.com/ros2/rmw_connextdds/pull/115
- [x] https://github.com/ros2/rmw_cyclonedds/pull/451
- [x] https://github.com/ros2/rmw_fastrtps/pull/687

TODOs

I might not get all of them, but wow am I going to try: (Note: Edits in each lower level struct will need changes propagated upwards, but the higher level structs are what users are more likely to interact with.)

[x] Message Type Support
- [x] rosidl_message_type_support_t
- [x] rosidl_dynamic_message_type_support_impl_t
[x] Dynamic Type Support
- [x] rosidl_dynamic_typesupport_serialization_support_t
  - getting this in means all dynamic typesupport types will have access to a user-configured allocator via their composed serialization supports (but doesn't yet unlock the ability to have independent allocators from the serialization support)
- [x] rosidl_dynamic_typesupport_dynamic_type_t
- [x] rosidl_dynamic_typesupport_dynamic_type_builder_t
- [x] rosidl_dynamic_typesupport_dynamic_data_t
[x] ~~Dynamic Type Support Impls (Do these even make sense to take allocators? :thinking: I don't think I will support them)~~
- [ ] ~~rosidl_dynamic_typesupport_serialization_support_impl_t~~
- [ ] ~~rosidl_dynamic_typesupport_dynamic_type_impl_t~~
- [ ] ~~rosidl_dynamic_typesupport_dynamic_type_builder_impl_t~~
- [ ] ~~rosidl_dynamic_typesupport_dynamic_data_impl_t~~
[ ] Type Description Utils
- This is a whole bunch of stuff... I might not get to this
[ ] ~~rclcpp (I'm not even going to think about this one yet)~~
- But for my reference... https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/subscription_options.hpp

Apr 07 '23 07:04 methylDragon

Linux
Linux-aarch64
Windows

Apr 09 '23 22:04 methylDragon

Linux
Linux-aarch64
Windows

Apr 10 '23 04:04 methylDragon

I'm gonna call this feature done! It's running fine and there aren't any memory/runtime issues that I can see.

:warning: : It's paramount to merge this because this significantly cleans up the memory management (pointers are replaced with values wherever possible), and also allows for users to use custom allocators with every struct in the dynamic types support stack.

Valgrind Logs of the Cleanup Come Up COMPLETELY CLEAN (except for pre-existing bugs in FastRTPS) (well.. I guess and some leaks)

The Valgrind Cleanup Logs

^C[INFO] [1681132282.322891325] [rclcpp]: signal_handler(signum=2)
==200582== Invalid read of size 1
==200582==    at 0x4FE54D4: eprosima::fastrtps::types::DynamicType::has_children() const (DynamicType.cpp:464)
==200582==    by 0x4FCB4C6: eprosima::fastrtps::types::DynamicData::clean_members() (DynamicData.cpp:691)
==200582==    by 0x4FCB61E: eprosima::fastrtps::types::DynamicData::clean() (DynamicData.cpp:636)
==200582==    by 0x4FCB6C1: eprosima::fastrtps::types::DynamicData::~DynamicData() (DynamicData.cpp:142)
==200582==    by 0x4FE2609: eprosima::fastrtps::types::DynamicDataFactory::delete_data(eprosima::fastrtps::types::DynamicData*) (DynamicDataFactory.cpp:215)
==200582==    by 0x777EAF5: fastrtps__dynamic_data_fini(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_data_impl_s*) (fastrtps_dynamic_data.cpp:299)
==200582==    by 0x49D5B74: rclcpp::dynamic_typesupport::DynamicMessage::~DynamicMessage() (dynamic_message.cpp:174)
==200582==    by 0x49DE09E: _M_release (shared_ptr_base.h:168)
==200582==    by 0x49DE09E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x49DE09E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x49DE09E: reset (shared_ptr_base.h:1272)
==200582==    by 0x49DE09E: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::~DynamicMessageTypeSupport() (dynamic_message_type_support.cpp:228)
==200582==    by 0x1126D1: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:168)
==200582==    by 0x11078E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x11078E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x11078E: ~shared_ptr (shared_ptr.h:122)
==200582==    by 0x11078E: main (dynamic_sub_serialized.cpp:117)
==200582==  Address 0xccb36f0 is 144 bytes inside a block of size 152 free'd
==200582==    at 0x484F8AF: operator delete(void*) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==200582==    by 0x4FF3705: eprosima::fastrtps::types::DynamicTypeBuilderFactory::delete_type(eprosima::fastrtps::types::DynamicType*) (DynamicTypeBuilderFactory.cpp:823)
==200582==    by 0x7782FAA: fastrtps__dynamic_type_fini(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_type_impl_s*) (fastrtps_dynamic_type.cpp:235)
==200582==    by 0x49D99E1: rclcpp::dynamic_typesupport::DynamicMessageType::~DynamicMessageType() (dynamic_message_type.cpp:109)
==200582==    by 0x49DE031: _M_release (shared_ptr_base.h:168)
==200582==    by 0x49DE031: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x49DE031: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x49DE031: reset (shared_ptr_base.h:1272)
==200582==    by 0x49DE031: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::~DynamicMessageTypeSupport() (dynamic_message_type_support.cpp:227)
==200582==    by 0x1126D1: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:168)
==200582==    by 0x11078E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x11078E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x11078E: ~shared_ptr (shared_ptr.h:122)
==200582==    by 0x11078E: main (dynamic_sub_serialized.cpp:117)
==200582==  Block was alloc'd at
==200582==    at 0x484D013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==200582==    by 0x4FF521C: eprosima::fastrtps::types::DynamicTypeBuilderFactory::create_type(eprosima::fastrtps::types::DynamicTypeBuilder const*) (DynamicTypeBuilderFactory.cpp:180)
==200582==    by 0x4FEDDBD: eprosima::fastrtps::types::DynamicTypeBuilder::build() (DynamicTypeBuilder.cpp:302)
==200582==    by 0x7783428: fastrtps__dynamic_type_init_from_dynamic_type_builder(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_type_builder_impl_s*, rcutils_allocator_s*, rosidl_dynamic_typesupport_dynamic_type_impl_s*) (fastrtps_dynamic_type.cpp:174)
==200582==    by 0x54BF952: rosidl_dynamic_typesupport_dynamic_type_init_from_dynamic_type_builder (dynamic_type.c:902)
==200582==    by 0x54C4A35: rosidl_dynamic_typesupport_dynamic_type_init_from_description (dynamic_type.c:945)
==200582==    by 0x54C4EE0: rosidl_dynamic_message_type_support_handle_impl_init (dynamic_message_type_support_struct.c:192)
==200582==    by 0x54C52CC: rosidl_dynamic_message_type_support_handle_init (dynamic_message_type_support_struct.c:67)
==200582==    by 0x547CC87: rcl_dynamic_message_type_support_handle_init (dynamic_message_type_support.c:91)
==200582==    by 0x49DEAEA: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::DynamicMessageTypeSupport(rosidl_runtime_c__type_description__TypeDescription const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rcutils_allocator_s) (dynamic_message_type_support.cpp:60)
==200582==    by 0x1128D8: construct<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (new_allocator.h:162)
==200582==    by 0x1128D8: construct<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (alloc_traits.h:516)
==200582==    by 0x1128D8: _Sp_counted_ptr_inplace<rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr_base.h:519)
==200582==    by 0x1128D8: __shared_count<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr_base.h:650)
==200582==    by 0x1128D8: std::__shared_ptr<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&>(std::_Sp_alloc_shared_tag<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport> >, rosidl_runtime_c__type_description__TypeDescription&) (shared_ptr_base.h:1342)
==200582==    by 0x110373: shared_ptr<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:409)
==200582==    by 0x110373: allocate_shared<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:863)
==200582==    by 0x110373: make_shared<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:879)
==200582==    by 0x110373: make_shared<rosidl_runtime_c__type_description__TypeDescription&> (dynamic_message_type_support.hpp:62)
==200582==    by 0x110373: main (dynamic_sub_serialized.cpp:99)
==200582== 
==200582== Invalid read of size 1
==200582==    at 0x4FCB407: eprosima::fastrtps::types::DynamicData::get_kind() const (DynamicData.cpp:460)
==200582==    by 0x4FCB547: eprosima::fastrtps::types::DynamicData::clean_members() (DynamicData.cpp:700)
==200582==    by 0x4FCB61E: eprosima::fastrtps::types::DynamicData::clean() (DynamicData.cpp:636)
==200582==    by 0x4FCB6C1: eprosima::fastrtps::types::DynamicData::~DynamicData() (DynamicData.cpp:142)
==200582==    by 0x4FE2609: eprosima::fastrtps::types::DynamicDataFactory::delete_data(eprosima::fastrtps::types::DynamicData*) (DynamicDataFactory.cpp:215)
==200582==    by 0x777EAF5: fastrtps__dynamic_data_fini(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_data_impl_s*) (fastrtps_dynamic_data.cpp:299)
==200582==    by 0x49D5B74: rclcpp::dynamic_typesupport::DynamicMessage::~DynamicMessage() (dynamic_message.cpp:174)
==200582==    by 0x49DE09E: _M_release (shared_ptr_base.h:168)
==200582==    by 0x49DE09E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x49DE09E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x49DE09E: reset (shared_ptr_base.h:1272)
==200582==    by 0x49DE09E: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::~DynamicMessageTypeSupport() (dynamic_message_type_support.cpp:228)
==200582==    by 0x1126D1: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:168)
==200582==    by 0x11078E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x11078E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x11078E: ~shared_ptr (shared_ptr.h:122)
==200582==    by 0x11078E: main (dynamic_sub_serialized.cpp:117)
==200582==  Address 0xccb36f0 is 144 bytes inside a block of size 152 free'd
==200582==    at 0x484F8AF: operator delete(void*) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==200582==    by 0x4FF3705: eprosima::fastrtps::types::DynamicTypeBuilderFactory::delete_type(eprosima::fastrtps::types::DynamicType*) (DynamicTypeBuilderFactory.cpp:823)
==200582==    by 0x7782FAA: fastrtps__dynamic_type_fini(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_type_impl_s*) (fastrtps_dynamic_type.cpp:235)
==200582==    by 0x49D99E1: rclcpp::dynamic_typesupport::DynamicMessageType::~DynamicMessageType() (dynamic_message_type.cpp:109)
==200582==    by 0x49DE031: _M_release (shared_ptr_base.h:168)
==200582==    by 0x49DE031: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x49DE031: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x49DE031: reset (shared_ptr_base.h:1272)
==200582==    by 0x49DE031: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::~DynamicMessageTypeSupport() (dynamic_message_type_support.cpp:227)
==200582==    by 0x1126D1: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:168)
==200582==    by 0x11078E: ~__shared_count (shared_ptr_base.h:705)
==200582==    by 0x11078E: ~__shared_ptr (shared_ptr_base.h:1154)
==200582==    by 0x11078E: ~shared_ptr (shared_ptr.h:122)
==200582==    by 0x11078E: main (dynamic_sub_serialized.cpp:117)
==200582==  Block was alloc'd at
==200582==    at 0x484D013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==200582==    by 0x4FF521C: eprosima::fastrtps::types::DynamicTypeBuilderFactory::create_type(eprosima::fastrtps::types::DynamicTypeBuilder const*) (DynamicTypeBuilderFactory.cpp:180)
==200582==    by 0x4FEDDBD: eprosima::fastrtps::types::DynamicTypeBuilder::build() (DynamicTypeBuilder.cpp:302)
==200582==    by 0x7783428: fastrtps__dynamic_type_init_from_dynamic_type_builder(rosidl_dynamic_typesupport_serialization_support_impl_s*, rosidl_dynamic_typesupport_dynamic_type_builder_impl_s*, rcutils_allocator_s*, rosidl_dynamic_typesupport_dynamic_type_impl_s*) (fastrtps_dynamic_type.cpp:174)
==200582==    by 0x54BF952: rosidl_dynamic_typesupport_dynamic_type_init_from_dynamic_type_builder (dynamic_type.c:902)
==200582==    by 0x54C4A35: rosidl_dynamic_typesupport_dynamic_type_init_from_description (dynamic_type.c:945)
==200582==    by 0x54C4EE0: rosidl_dynamic_message_type_support_handle_impl_init (dynamic_message_type_support_struct.c:192)
==200582==    by 0x54C52CC: rosidl_dynamic_message_type_support_handle_init (dynamic_message_type_support_struct.c:67)
==200582==    by 0x547CC87: rcl_dynamic_message_type_support_handle_init (dynamic_message_type_support.c:91)
==200582==    by 0x49DEAEA: rclcpp::dynamic_typesupport::DynamicMessageTypeSupport::DynamicMessageTypeSupport(rosidl_runtime_c__type_description__TypeDescription const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rcutils_allocator_s) (dynamic_message_type_support.cpp:60)
==200582==    by 0x1128D8: construct<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (new_allocator.h:162)
==200582==    by 0x1128D8: construct<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (alloc_traits.h:516)
==200582==    by 0x1128D8: _Sp_counted_ptr_inplace<rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr_base.h:519)
==200582==    by 0x1128D8: __shared_count<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr_base.h:650)
==200582==    by 0x1128D8: std::__shared_ptr<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&>(std::_Sp_alloc_shared_tag<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport> >, rosidl_runtime_c__type_description__TypeDescription&) (shared_ptr_base.h:1342)
==200582==    by 0x110373: shared_ptr<std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:409)
==200582==    by 0x110373: allocate_shared<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, std::allocator<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport>, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:863)
==200582==    by 0x110373: make_shared<rclcpp::dynamic_typesupport::DynamicMessageTypeSupport, rosidl_runtime_c__type_description__TypeDescription&> (shared_ptr.h:879)
==200582==    by 0x110373: make_shared<rosidl_runtime_c__type_description__TypeDescription&> (dynamic_message_type_support.hpp:62)
==200582==    by 0x110373: main (dynamic_sub_serialized.cpp:99)
==200582== 
2023-04-10 06:11:22.543 [DYN_TYPES Error] Error deleting DynamicData. It isn't registered in the factory -> Function delete_data
==200582== 
==200582== HEAP SUMMARY:
==200582==     in use at exit: 49,918 bytes in 158 blocks
==200582==   total heap usage: 29,479 allocs, 29,321 frees, 6,362,026 bytes allocated
==200582== 
==200582== LEAK SUMMARY:
==200582==    definitely lost: 268 bytes in 6 blocks
==200582==    indirectly lost: 64 bytes in 3 blocks
==200582==      possibly lost: 768 bytes in 2 blocks
==200582==    still reachable: 48,818 bytes in 147 blocks
==200582==         suppressed: 0 bytes in 0 blocks
==200582== Rerun with --leak-check=full to see details of leaked memory

Linux (The agent disconnected, the failure isn't due to the PRs)
Linux-aarch64
Windows
Windows Debug
RHEL

(I'm expecting one lint failure that I fixed post-hoc)

Apr 10 '23 13:04 methylDragon

Run 2 for posterity: (these are unstable only because of uncrustify!!)

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

Apr 10 '23 13:04 methylDragon

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

Apr 10 '23 16:04 methylDragon

Linux
Linux-aarch64
Windows

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

Apr 10 '23 20:04 methylDragon

After discussion, the future work here is as follows:

Get https://github.com/ros2/rclcpp/pull/2160 reviewed and merged into Rolling.
Get https://github.com/ros2/rclcpp/pull/2160 backported to Iron for a patch release
Implement dynamic type support for Connext (probably needs a new rosidl_dynamic_typesupport_connext package).
Update rmw_connextdds to take advantage of the above.
Implement dynamic type support for CycloneDDS (probably needs a new rosidl_dynamic_typesupport_cyclonedds package).
Update rmw_cyclonedds to take advantage of the above.
Documentation and examples for the feature.

What I'll suggest is that after we get the first two items done, we close out this issue and open a new one to track the remaining work.

May 02 '23 21:05 clalancette

ros2
ros2 copied to clipboard

Dynamic Subscription (REP-2011 subset): BONUS: Allocator Refactor

Description

NOTE

Relevant PRs

TODOs

ros2 ros2 copied to clipboard

Dynamic Subscription (REP-2011 subset): BONUS: Allocator Refactor

Description

NOTE

Relevant PRs

TODOs

ros2
ros2 copied to clipboard