From 1c63e7fd9469cc14710e83157c75bfb7754c47a1 Mon Sep 17 00:00:00 2001 From: Steve Wolter Date: Tue, 22 Sep 2020 08:18:37 +0000 Subject: [PATCH] Make get_rcl_allocator a function family As explained in #1254, there's conceptually no way to implement RCL allocators in terms of C++ allocators. In order to fix this behavior, we have to delete the generic version of get_rcl_allocator. Since some ROS code depends on this, we need to allow users to write their own version of get_rcl_allocator for allocators that support the C-style interface (most do). So this CL changes get_rcl_allocator from a template function into a family of (potentially templated) functions, which allows users to add their own overloads and rely on the "most specialized" mechanism for function specialization to select the right one. See http://www.gotw.ca/publications/mill17.htm for details. This also allows us to return get_rcl_default_allocator for all specializations of std::allocator (previously, only for std::allocator), which will already fix #1254 for pretty much all clients. I'll continue to work on deleting the generic version, though, to make sure that nobody is accidentally bitten by it. I've tried to test this by doing a full ROS compilation following the Dockerfile of the source Docker image, and all packages compile. Signed-off-by: Steve Wolter Addressed code style test failures. Signed-off-by: Steve Wolter Update comments. Incidentally, this also reruns the flaky test suite, which seems to be using the real DDS middleware and to be prone to cross-talk. Signed-off-by: Steve Wolter Also update recently added tests. Signed-off-by: Steve Wolter Added reference to bug number. Signed-off-by: Steve Wolter Extend allocator lifetime in options. Signed-off-by: Steve Wolter Drop the generic version of get_rcl_allocator. This allows us to simplify a bunch of code as well. Signed-off-by: Steve Wolter Update rclcpp/include/rclcpp/allocator/allocator_common.hpp Co-authored-by: William Woodall Signed-off-by: Steve Wolter Revert accidental deletion of allocator creation. Signed-off-by: Steve Wolter --- .../rclcpp/allocator/allocator_common.hpp | 74 +++-------------- .../rclcpp/message_memory_strategy.hpp | 5 +- rclcpp/include/rclcpp/publisher_options.hpp | 9 ++- .../strategies/allocator_memory_strategy.hpp | 2 +- .../include/rclcpp/subscription_options.hpp | 9 ++- .../allocator/test_allocator_common.cpp | 79 +++---------------- 6 files changed, 37 insertions(+), 141 deletions(-) diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 12b2f383b6..f1d411d25f 100644 --- a/rclcpp/include/rclcpp/allocator/allocator_common.hpp +++ b/rclcpp/include/rclcpp/allocator/allocator_common.hpp @@ -30,14 +30,19 @@ namespace allocator template using AllocRebind = typename std::allocator_traits::template rebind_traits; -template -void * retyped_allocate(size_t size, void * untyped_allocator) +/// Return the equivalent rcl_allocator_t for the C++ standard allocator. +/** + * This assumes that the user intent behind both allocators is the + * same: Using system malloc for allocation. + * + * If you're using a custom allocator in ROS, you'll need to provide + * your own overload for this function. + */ +template +rcl_allocator_t get_rcl_allocator(std::allocator allocator) { - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - return std::allocator_traits::allocate(*typed_allocator, size); + (void)allocator; // Remove warning + return rcl_get_default_allocator(); } template @@ -56,61 +61,6 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void * return allocated_memory; } -template -void retyped_deallocate(void * untyped_pointer, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); -} - -template -void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); - return std::allocator_traits::allocate(*typed_allocator, size); -} - - -// Convert a std::allocator_traits-formatted Allocator into an rcl allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::value>::type * = nullptr> -rcl_allocator_t get_rcl_allocator(Alloc & allocator) -{ - rcl_allocator_t rcl_allocator = rcl_get_default_allocator(); -#ifndef _WIN32 - rcl_allocator.allocate = &retyped_allocate; - rcl_allocator.zero_allocate = &retyped_zero_allocate; - rcl_allocator.deallocate = &retyped_deallocate; - rcl_allocator.reallocate = &retyped_reallocate; - rcl_allocator.state = &allocator; -#else - (void)allocator; // Remove warning -#endif - return rcl_allocator; -} - -// TODO(jacquelinekay) Workaround for an incomplete implementation of std::allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::value>::type * = nullptr> -rcl_allocator_t get_rcl_allocator(Alloc & allocator) -{ - (void)allocator; - return rcl_get_default_allocator(); -} - } // namespace allocator } // namespace rclcpp diff --git a/rclcpp/include/rclcpp/message_memory_strategy.hpp b/rclcpp/include/rclcpp/message_memory_strategy.hpp index f548d953c2..d664239b25 100644 --- a/rclcpp/include/rclcpp/message_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/message_memory_strategy.hpp @@ -18,6 +18,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/types.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -61,7 +62,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(); serialized_message_allocator_ = std::make_shared(); buffer_allocator_ = std::make_shared(); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } explicit MessageMemoryStrategy(std::shared_ptr allocator) @@ -69,7 +70,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(*allocator.get()); serialized_message_allocator_ = std::make_shared(*allocator.get()); buffer_allocator_ = std::make_shared(*allocator.get()); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } virtual ~MessageMemoryStrategy() = default; diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 3c88ebccd1..04d943265d 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -20,6 +20,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/publisher.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -77,7 +78,8 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase /// Constructor using base class as input. explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base) : PublisherOptionsBase(publisher_options_base) - {} + { + } /// Convert this class, and a rclcpp::QoS, into an rcl_publisher_options_t. template @@ -85,7 +87,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase to_rcl_publisher_options(const rclcpp::QoS & qos) const { rcl_publisher_options_t result = rcl_publisher_get_default_options(); - result.allocator = this->get_rcl_allocator(); + result.allocator = rcl_get_default_allocator(); result.qos = qos.get_rmw_qos_profile(); result.rmw_publisher_options.require_unique_network_flow_endpoints = this->require_unique_network_flow_endpoints; @@ -98,7 +100,6 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase return result; } - /// Get the allocator, creating one if needed. std::shared_ptr get_allocator() const @@ -123,7 +124,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase plain_allocator_storage_ = std::make_shared(*this->get_allocator()); } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + return rcl_get_default_allocator(); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp index 88698179d4..db8a59f5c7 100644 --- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp @@ -424,7 +424,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy rcl_allocator_t get_allocator() override { - return rclcpp::allocator::get_rcl_allocator(*allocator_.get()); + return rcl_get_default_allocator(); } size_t number_of_ready_subscriptions() const override diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 2b819da399..5f9dbf5ec7 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -21,6 +21,8 @@ #include #include +#include "rcl/allocator.h" + #include "rclcpp/callback_group.hpp" #include "rclcpp/detail/rmw_implementation_specific_subscription_payload.hpp" #include "rclcpp/intra_process_buffer_type.hpp" @@ -103,7 +105,8 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase explicit SubscriptionOptionsWithAllocator( const SubscriptionOptionsBase & subscription_options_base) : SubscriptionOptionsBase(subscription_options_base) - {} + { + } /// Convert this class, with a rclcpp::QoS, into an rcl_subscription_options_t. /** @@ -115,7 +118,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase to_rcl_subscription_options(const rclcpp::QoS & qos) const { rcl_subscription_options_t result = rcl_subscription_get_default_options(); - result.allocator = this->get_rcl_allocator(); + result.allocator = rcl_get_default_allocator(); result.qos = qos.get_rmw_qos_profile(); result.rmw_subscription_options.ignore_local_publications = this->ignore_local_publications; result.rmw_subscription_options.require_unique_network_flow_endpoints = @@ -167,7 +170,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase plain_allocator_storage_ = std::make_shared(*this->get_allocator()); } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + return rcl_get_default_allocator(); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp index 4619b7665d..5e1d8bc90c 100644 --- a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp +++ b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp @@ -18,54 +18,17 @@ #include "rclcpp/allocator/allocator_common.hpp" -TEST(TestAllocatorCommon, retyped_allocate) { - std::allocator allocator; - void * untyped_allocator = &allocator; - void * allocated_mem = - rclcpp::allocator::retyped_allocate>(1u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); - - allocated_mem = allocator.allocate(1); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - void * reallocated_mem = - rclcpp::allocator::retyped_reallocate>( - allocated_mem, 2u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != reallocated_mem); - - auto code2 = [&untyped_allocator, reallocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); -} - -TEST(TestAllocatorCommon, retyped_zero_allocate_basic) { +TEST(TestAllocatorCommon, retyped_zero_allocate_basic) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = rclcpp::allocator::retyped_zero_allocate>(20u, 1u, untyped_allocator); ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); } -TEST(TestAllocatorCommon, retyped_zero_allocate) { +TEST(TestAllocatorCommon, retyped_zero_allocate) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = @@ -73,34 +36,12 @@ TEST(TestAllocatorCommon, retyped_zero_allocate) { // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); - - allocated_mem = allocator.allocate(1); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - void * reallocated_mem = - rclcpp::allocator::retyped_reallocate>( - allocated_mem, 2u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != reallocated_mem); - - auto code2 = [&untyped_allocator, reallocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); } -TEST(TestAllocatorCommon, get_rcl_allocator) { +TEST(TestAllocatorCommon, get_rcl_allocator) +{ std::allocator allocator; - auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); + auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); EXPECT_NE(nullptr, rcl_allocator.allocate); EXPECT_NE(nullptr, rcl_allocator.deallocate); EXPECT_NE(nullptr, rcl_allocator.reallocate); @@ -108,10 +49,10 @@ TEST(TestAllocatorCommon, get_rcl_allocator) { // Not testing state as that may or may not be null depending on platform } -TEST(TestAllocatorCommon, get_void_rcl_allocator) { +TEST(TestAllocatorCommon, get_void_rcl_allocator) +{ std::allocator allocator; - auto rcl_allocator = - rclcpp::allocator::get_rcl_allocator>(allocator); + auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); EXPECT_NE(nullptr, rcl_allocator.allocate); EXPECT_NE(nullptr, rcl_allocator.deallocate); EXPECT_NE(nullptr, rcl_allocator.reallocate);