Skip to content

Commit

Permalink
Keep custom allocator in publisher and subscription options alive. (r…
Browse files Browse the repository at this point in the history
…os2#1647)

* Keep custom allocator in publisher and subscription options alive.

Also, enforce an allocator bound to void is used to avoid surprises.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Avoid sizeof(void) in MyAllocator implementation.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Address peer review comment

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Use a lazely initialized private field when 'allocator' is not initialized

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
  • Loading branch information
hidmic and ivanpauno authored May 4, 2021
1 parent c15eb1e commit 1fc2d58
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 12 deletions.
20 changes: 15 additions & 5 deletions rclcpp/include/rclcpp/publisher_options.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <memory>
#include <string>
#include <type_traits>
#include <vector>

#include "rcl/publisher.h"
Expand Down Expand Up @@ -64,6 +65,10 @@ struct PublisherOptionsBase
template<typename Allocator>
struct PublisherOptionsWithAllocator : public PublisherOptionsBase
{
static_assert(
std::is_void_v<typename std::allocator_traits<Allocator>::value_type>,
"Publisher allocator value type must be void");

/// Optional custom allocator.
std::shared_ptr<Allocator> allocator = nullptr;

Expand All @@ -80,10 +85,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
to_rcl_publisher_options(const rclcpp::QoS & qos) const
{
rcl_publisher_options_t result = rcl_publisher_get_default_options();
using AllocatorTraits = std::allocator_traits<Allocator>;
using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
auto message_alloc = std::make_shared<MessageAllocatorT>(*this->get_allocator().get());
result.allocator = rclcpp::allocator::get_rcl_allocator<MessageT>(*message_alloc);
result.allocator = rclcpp::allocator::get_rcl_allocator<void>(*this->get_allocator());
result.qos = qos.get_rmw_qos_profile();
result.rmw_publisher_options.require_unique_network_flow_endpoints =
this->require_unique_network_flow_endpoints;
Expand All @@ -102,10 +104,18 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
get_allocator() const
{
if (!this->allocator) {
return std::make_shared<Allocator>();
if (!allocator_storage_) {
allocator_storage_ = std::make_shared<Allocator>();
}
return allocator_storage_;
}
return this->allocator;
}

private:
// This is a temporal workaround, to make sure that get_allocator()
// always returns a copy of the same allocator.
mutable std::shared_ptr<Allocator> allocator_storage_;
};

using PublisherOptions = PublisherOptionsWithAllocator<std::allocator<void>>;
Expand Down
21 changes: 15 additions & 6 deletions rclcpp/include/rclcpp/subscription_options.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <chrono>
#include <memory>
#include <string>
#include <type_traits>
#include <vector>

#include "rclcpp/callback_group.hpp"
Expand Down Expand Up @@ -86,6 +87,10 @@ struct SubscriptionOptionsBase
template<typename Allocator>
struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
{
static_assert(
std::is_void_v<typename std::allocator_traits<Allocator>::value_type>,
"Subscription allocator value type must be void");

/// Optional custom allocator.
std::shared_ptr<Allocator> allocator = nullptr;

Expand All @@ -107,10 +112,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
to_rcl_subscription_options(const rclcpp::QoS & qos) const
{
rcl_subscription_options_t result = rcl_subscription_get_default_options();
using AllocatorTraits = std::allocator_traits<Allocator>;
using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
auto message_alloc = std::make_shared<MessageAllocatorT>(*this->get_allocator().get());
result.allocator = allocator::get_rcl_allocator<MessageT>(*message_alloc);
result.allocator = allocator::get_rcl_allocator<void>(*this->get_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 =
Expand All @@ -124,15 +126,22 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
return result;
}

/// Get the allocator, creating one if needed.
std::shared_ptr<Allocator>
get_allocator() const
{
if (!this->allocator) {
return std::make_shared<Allocator>();
if (!allocator_storage_) {
allocator_storage_ = std::make_shared<Allocator>();
}
return allocator_storage_;
}
return this->allocator;
}

private:
// This is a temporal workaround, to make sure that get_allocator()
// always returns a copy of the same allocator.
mutable std::shared_ptr<Allocator> allocator_storage_;
};

using SubscriptionOptions = SubscriptionOptionsWithAllocator<std::allocator<void>>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <list>
#include <memory>
#include <string>
#include <type_traits>
#include <utility>

#include "test_msgs/msg/empty.hpp"
Expand Down Expand Up @@ -57,7 +58,10 @@ struct MyAllocator
return nullptr;
}
num_allocs++;
return static_cast<T *>(std::malloc(size * sizeof(T)));
// Use sizeof(char) in place for sizeof(void)
constexpr size_t value_size = sizeof(
typename std::conditional<!std::is_void<T>::value, T, char>::type);
return static_cast<T *>(std::malloc(size * value_size));
}

void deallocate(T * ptr, size_t size)
Expand Down

0 comments on commit 1fc2d58

Please sign in to comment.