Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Keep custom allocator in publisher and subscription options alive. #1647

Merged
merged 4 commits into from
May 4, 2021
Merged

Keep custom allocator in publisher and subscription options alive. #1647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f768bb
Keep custom allocator in publisher and subscription options alive.
hidmic Apr 26, 2021
f4dcd4e
Avoid sizeof(void) in MyAllocator implementation.
hidmic Apr 29, 2021
6ff1246
Address peer review comment
ivanpauno Apr 30, 2021
fe13993
Use a lazely initialized private field when 'allocator' is not initia…
ivanpauno May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions rclcpp/include/rclcpp/publisher_options.hpp
View file
Open in desktop
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());
Copy link
Contributor Author

@hidmic hidmic May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this was a mistake. From cppreference:

The explicit specialization for void lacks the member typedefs reference, const_reference, size_type and difference_type. This specialization declares no member functions.

It appears void allocators are not supposed to be used for anything but type-erasure. If everything complies with that i.e. no code attempts to call allocate on a void allocator, then you can afford not making the explicit specialization like the test allocator, the allocator tutorial and now the tlsf allocator do...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you fix that one?

Copy link
Contributor Author

@hidmic hidmic May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tlsf_cpp? We could, but I'm starting to think we shouldn't be asking custom void allocators to have char-like allocate/deallocate APIs. Even if they were written carelessly (with no explicit specialization for void) and have them, chances are it won't build. See #1657.

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
View file
Open in desktop
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
6 changes: 5 additions & 1 deletion rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine. I spent some time looking at libcxx's code and I couldn't find anything in the void specialization that avoids sizeof(void), but I think that's technically a GCC extension. I wondered if uint8_t is better than char here (even though they are functionally the same), and I was actually looking for what they did when I couldn't find logic similar to this in their allocate function for std::allocator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually hoping to find something on the std::allocator class or the std::allocator_traits that we could use instead, e.g. something like std::allocator<T>::element_size, but it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the tlsf_cpp allocator in realtime_support is showing the same issue https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/778/console#console-section-24.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a new warning in all builds, likely related to this: https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_debug/1593/gcc/new/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same root cause.

return static_cast<T *>(std::malloc(size * value_size));
}

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