[RPC][BUGFIX][BACKPORT-0.6] Fix bug in rpc ring buffer shrink #5516
Conversation
src/support/ring_buffer.h
Outdated
| size_t old_bytes = bytes_available_; | ||
|
|
||
| std::vector<char> tmp(old_bytes); | ||
|
|
||
| Read(&tmp[0], old_bytes); | ||
| ring_.resize(kInitCapacity); | ||
| if (old_bytes != 0) { | ||
| Read(&tmp[0], old_bytes); | ||
| } | ||
|
|
||
| size_t new_size = kInitCapacity; | ||
| new_size = std::max(new_size, bytes_available_); | ||
| new_size = std::max(new_size, n); | ||
|
|
||
| ring_.resize(new_size); | ||
| ring_.shrink_to_fit(); | ||
|
|
||
| memcpy(&ring_[0], &tmp[0], old_bytes); | ||
| if (old_bytes != 0) { | ||
| memcpy(&ring_[0], &tmp[0], old_bytes); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not performance critical but I think it could be better recast as below.
if (bytes_available != 0)
{
... existing code
}
else
{
ring_.resize(kInitCapacity);
ring_.shrink_to_fit();
}
Avoids unnecessary runtime calls to std::max with 0 and additional checks that can go. And for bonus points, encapsulate ring_.resize(new_size) and ring_.shrink_to_fit into a small helper function if you so choose.
There was a problem hiding this comment.
Thanks for the comment, we I updated the content to make it a bit more compact, we still need to compare to n in case n is larger than initcapacity
There was a problem hiding this comment.
I see, yes of-course I can now see that capacity can be shrunk to max (kinitCapacity, n, bytes_available). Might be worth a small comment.
regards
Ramana
|
tag @FrozenGene who also met this problem |
cc @merrymercy @giuseros @ZihengJiang @areusch @tmoreau89