ros2 / demos

Apache License 2.0
491 stars 329 forks source link

Replacing old-style C++ allocator with a polymorphic memory resource (PMR) #653

Closed AliAshkaniNia closed 10 months ago

AliAshkaniNia commented 10 months ago

This is a basic example mimicking the original demo. We could use a more advanced resource, like unsynchronized_pool_resource, for a realistic scenario.

AliAshkaniNia commented 10 months ago

This is a change for issue #650 .

AliAshkaniNia commented 10 months ago

My apologies, I should have read the ROS2 developer guides before making changes. Now, it is compliant with ROS2 style, and I have also reordered the #include statements.

clalancette commented 10 months ago

I am not sure about this, we could. but we would not want to introduce the polymorphic memory details here, instead we would like it to be simple for demonstration code.

Yeah, I agree. What we have here is easy to understand, and is the main point of this tutorial, so I think we should keep it simple.

clalancette commented 10 months ago

CI:

AliAshkaniNia commented 10 months ago

Thanks, this is much easier to understand.

Glad I could assist!

AliAshkaniNia commented 10 months ago

For the unsynchronized_pool_resource stuff, I agree with you. I'm a fan of making tutorials dead simple, which is why I came up with this refactoring idea. Please correct me if I'm wrong, but on the other hand, my perception is that ROS2 comes with "batteries included"; it provides all the necessary tools and resources for creating production-ready code. I believe there's a good chance that someone interested in custom allocators might also be interested in using stack memory instead of heap. So, I thought it's worth mentioning that topic thanks to the intuitive API:

std::array<std::uint8_t, 10*1024> buffer{};
std::pmr::monotonic_buffer_resource upstream_resource(buffer.data(), buffer.size());
std::pmr::unsynchronized_pool_resource mem_resource(&upstream_resource);

auto alloc = std::make_shared<Alloc>(&mem_resource);

Again, this is just a suggestion, and overall, I agree that it might be too complicated for the tutorial. I just mentioned it for the sake of completeness.