Skip to content

Reuse FastBuffer managed buffer for each endpoint. - #904

Open
dskkato wants to merge 1 commit into
ros2:rollingfrom
dskkato:dskkato/reduce_extra_heap_allocations
Open

Reuse FastBuffer managed buffer for each endpoint.#904
dskkato wants to merge 1 commit into
ros2:rollingfrom
dskkato:dskkato/reduce_extra_heap_allocations

Conversation

@dskkato

@dskkato dskkato commented Aug 11, 2026

Copy link
Copy Markdown

Description

Previously, heap allocation happend for each endpoint with std::vector<uint8_t>. This introduced extra memset via its constructor.

For recent ROS 2, where rosidl::Buffer was introduced, sometimes the actual payload size and message size may not match because only buffer descriptors may be sent. In such a case, now get_serialized_size() seems to be unreliable, and hard to pre-allocate the buffer in that path.

Fixes #903

Is this user-facing behavior change?

Possibly yes.

Previously, it is guaranteed that the heap allocation only happens once per endpoint, through externally managed buffer (allocated with std::vector<uint8_t>). Now the number of allocations depends on combinations of buffer's strategies, and Cdr's usage. So, in small message cases, this patch will introduce an extra overhead of buffer reallocation.

For rosidl::Buffer usage, this patch will dramatically reduce extra allocation and zero-initialization when only the optimized message paths exist.

Did you use Generative AI?

I used CodeX to investigate the cause analysis, and FastBuffer/Cdr behavior.

Additional Information

This patch is trying to resolve two issues at same time:

  1. Reducing extra memory allocation when publishing rosidl::Buffer via optimized buf (carries only a descriptor)
  2. Removing extra memset caused by std::vector<uint8_t> constructor.

What I originally found is the second point, so if we wish more conservative way, it is possible to reserve the fast_buffer as previouslly. Still, I believe it is better to reuse that buffer accross endpoints, in my perspective...

Since this is my first PR to rmw_fastrtps repository, let me know if I missed anything.

@dskkato
dskkato force-pushed the dskkato/reduce_extra_heap_allocations branch from 08d53a6 to 6e58ea5 Compare August 11, 2026 08:25
@dskkato
dskkato marked this pull request as ready for review August 11, 2026 11:18
@mergify

mergify Bot commented Aug 11, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Previously, heap allocation happend for each endpoint with
std::vector<uint8_t>. This introduced extra memset via its constructor.

For recent ROS 2, where rosidl::Buffer was introduced, sometimes the
actual payload and message doesn't match because only buffer descriptors
may be sent. In such a case, now `get_serialized_size()` seems to be
unreliable, and hard to pre-allocate the buffer in that path.

Signed-off-by: dskkato <kato.daisuke429@gmail.com>
@dskkato
dskkato force-pushed the dskkato/reduce_extra_heap_allocations branch from 6e58ea5 to 9426421 Compare August 11, 2026 13:14
@tfoote
tfoote requested a review from nvcyc August 11, 2026 15:45
@dskkato

dskkato commented Aug 15, 2026

Copy link
Copy Markdown
Author

I shared a result of this patch here for a reference.

https://discourse.openrobotics.org/t/has-a-cpu-shared-memory-backend-for-rosidl-buffer-been-explored/57240/7

@nvcyc nvcyc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR!
The fix looks good to me.

I also recently found the same allocation overhead issue from some benchmarks and I've verified that this PR does eliminate the unnecessary memory allocation overhead that grows with the underlying data size.

@nvcyc
nvcyc requested a review from hidmic August 20, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove an extra memset for buffer_data for cdr serialization

3 participants