-
Notifications
You must be signed in to change notification settings - Fork 43
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?
Conversation
The aim of this pull request is to fix high latency for relatively large topics (e.g. 1 MB dd82e84
401016c
401016c + eed223a
|
16df6fe
to
401016c
Compare
All right, now that we've merged in #327 , we can consider this one. Please rebase this onto the latest, then we can do a full review of it. Until then, I'll mark it as a draft. |
401016c
to
62cb09b
Compare
068cf50
to
21006d0
Compare
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.
There are also many changes unrelated with the goal of this PR
7ca544b
to
bb6fd88
Compare
There was a formatting error from my IDE. I've restored the files and manually re-applied the patches. |
8dd9bf5
to
bcc36a1
Compare
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.
Besides the comments inline, do you have any updated performance numbers here?
///============================================================================= | ||
BufferPool::~BufferPool() | ||
{ | ||
rcutils_allocator_t allocator = rcutils_get_default_allocator(); |
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
during rmw_init
. To get to that, we'll have to change the constructor of rmw_context_impl_s
to pass that into the BufferPool
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.
- According to the documentation,
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. - Other RMW implementations don't use
rmw_init_options_s::allocator
either. Instead they usercutils_get_default_allocator
andrmw_allocate
. - The only place where this allocator seems useful is in
rmw_init_options_fini
.
We also allocate many std::vector
and std::string
without the default allocator anyway, which seems wrong.
I can make a subsequent pull request addressing this issue.
The following results were obtained using the Mont Blanc topology (with a 1 MiB 57a6b4b
8ba5278
cebb972
cebb972 + 8ba5278
|
e7f140c
to
0015353
Compare
Adds a bounded LIFO buffer pool in the context to reuse buffers allocated on serialization. The aim is not (only) to avoid the overhead of dynamic allocation but rather to enhance the cache locality of serialization buffers.
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Mahmoud Mazouz <[email protected]>
0015353
to
6c48aa8
Compare
Adds a LIFO buffer pool in the context to reuse buffers allocated on serialization. The aim is not (only) to avoid the overhead of dynamic allocation but rather to enhance the cache locality of serialization buffers.