Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Recycle serialization buffers on transmission #342
base: rolling
Are you sure you want to change the base?
Recycle serialization buffers on transmission #342
Changes from all commits
75d7c88
48addf5
2fa2381
e54660f
d68369e
7f1438b
6781c42
06ff7a9
e5c70c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat pedantic, but I think this should be a WARN since we are continuing on anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to require some additional plumbing, but I think we should respect the allocator passed in via the
options
duringrmw_init
. To get to that, we'll have to change the constructor ofrmw_context_impl_s
to pass that into theBufferPool
constructor, and then we can store the pointer in this class and use it as necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be using the allocator passed in
rmw_init_options_s
for all allocation in the RMW.rmw_init_options_s::allocator
is "[The] allocator used during internal allocation of init options, if needed.". So by the samermw_init
is called, we have no business allocating through it.rmw_init_options_s::allocator
either. Instead they usercutils_get_default_allocator
andrmw_allocate
.rmw_init_options_fini
.We also allocate many
std::vector
andstd::string
without the default allocator anyway, which seems wrong.I can make a subsequent pull request addressing this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing with the allocators is that always using
rcutils_get_default_allocator
is useless. The default allocator is justmalloc
,realloc
,free
, etc. So you may as well use those.Memory allocation is a tricky subject here. The original goal of the
rcutils_allocator
was to make it so that consumers of the RMW API could add in an allocator that is not the default (like a pool allocator). Early on inrmw_zenoh
development, we were very careful to use the passed-in allocator everywhere. As we've switched to C++ in more places, that has somewhat fallen by the wayside.@Yadunund What's your thinking here? Should we give up on the
rcutils_allocator
and just use C++ builtins everywhere? Or should we move in the direction of trying to honorrcutils_allocator
as much as we can?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrongly under the impression that
rcutils_get_default_allocator
could be overridden. I don't mind doing the plumbing if we decide to use thercutils_allocator
passed inrmw_init
.I also think that the RMW documentation is misleading and should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer moving this into
zenoh_utils.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move into
Data
impl and add public method to class to return ashared_ptr<rmf_zenoh_cpp::BufferPool>