Make the new buffer impl's reserve/commit semantics match libevent#7079
Make the new buffer impl's reserve/commit semantics match libevent#7079mattklein123 merged 3 commits intoenvoyproxy:masterfrom brian-pane:buffer_reuse
Conversation
* On OwnedImpl::reserve, if any slices at the end of the buffer have reservations, clear the reservations so the space can be used for the new reservation. * Add tests for the two cases where this is important: calling reserve and never committing the buffers before the next reserve call; and calling reserve, getting back N slices, and only committing the first M of those slices (where M < N) before the next reserve call. Signed-off-by: Brian Pane <brianp+github@brianp.net>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. Quick question.
/wait-any
source/common/buffer/buffer_impl.cc
Outdated
| size_t first_reservable_slice = slices_.size(); | ||
| while (first_reservable_slice > 0) { | ||
| // Reclaim any space that was previously reserved but never committed. | ||
| slices_[first_reservable_slice - 1]->clearReservation(); |
There was a problem hiding this comment.
Probably stupid question: Why do we need to keep track of reservation state at all if we have these semantics? Can't you just return allocated but not used chain space if the user asks for it again?
There was a problem hiding this comment.
Yeah, with the new semantics, we probably can stop tracking the reservation state. I'll try making that change (which should simplify the implementation) later today.
… replaces the last. Signed-off-by: Brian Pane <brianp+github@brianp.net>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM with a small nit and question.
/wait
source/common/buffer/buffer_impl.h
Outdated
| } | ||
| return capacity_ - reservable_; | ||
| } | ||
| uint64_t reservableSize() const { return capacity_ - reservable_; } |
There was a problem hiding this comment.
nit: ASSERT(capacity_ >= reservable_); ?
source/common/buffer/buffer_impl.h
Outdated
| if (size == 0) { | ||
| return {nullptr, 0}; | ||
| } | ||
| if (data_ != 0 && data_ == reservable_) { |
There was a problem hiding this comment.
Q: Why would this branch happen vs. being reset in the drain path? Should this be an assert instead?
There was a problem hiding this comment.
For a while, I couldn't decide whether to reset the offsets in the drain path or the reserve path, so it ended up in both. I'll replace this one with an assert.
Signed-off-by: Brian Pane <brianp+github@brianp.net>
Description:
have reservations, clear the reservations so the space can be
used for the new reservation.
reserve and never committing the buffers before the next reserve
call; and calling reserve, getting back N slices, and only committing
the first M of those slices (where M < N) before the next reserve
call.
Risk Level: medium
Testing: new unit tests included
Docs Changes: N/A
Release Notes: N/A
Signed-off-by: Brian Pane brianp+github@brianp.net