GH-43949: [C++] io::BufferedInput: Fix invalid state after SetBufferSize #44387
Conversation
|
|
|
@pitrou @felipecrv @wgtmac This might be a bit critical fixing Also cc @biljazovic sorry for so slow replying |
cpp/src/arrow/io/buffered.cc
Outdated
| new_buffer_size = | ||
| std::min(new_buffer_size, buffer_size_ + (raw_read_bound_ - raw_read_total_)); |
There was a problem hiding this comment.
| new_buffer_size = | |
| std::min(new_buffer_size, buffer_size_ + (raw_read_bound_ - raw_read_total_)); | |
| new_buffer_size = | |
| std::min(new_buffer_size, buffer_pos_ + buffer_size_ + (raw_read_bound_ - raw_read_total_)); |
Isn't buffer_pos_ required to be kept as well?
There was a problem hiding this comment.
| buffer_size_ |
| buffer_pos_ |
There was a problem hiding this comment.
My bad! I have confused bytes_buffered_ with buffer_size_.
There was a problem hiding this comment.
but you're right, I've a bug here. buffer_size_ might greater than buffer_pos_ + bytes_buffered_.
github-actions
bot
added
awaiting committer review
|
Are you calling |
No, |
| @@ -111,6 +111,7 @@ class ARROW_EXPORT BufferedInputStream | |||
| int64_t raw_read_bound = -1); | |||
|
|
|||
| /// \brief Resize internal read buffer; calls to Read(...) will read at least | |||
| /// this many bytes from the raw InputStream if possible. | |||
There was a problem hiding this comment.
I don't quite understand what's the intent of at least in the previous comment since it is incomplete. I suppose it should be will read at most new_buffer_size?
|
@pitrou would you mind take a look when you have spare time? Thanks! |
|
I'll merge it this week if no negative comment since it's a bugfix. Some comment can be address after bug is fixed |
cpp/src/arrow/io/buffered.cc
Outdated
| @@ -311,7 +326,7 @@ class BufferedInputStream::Impl : public BufferedBase { | |||
| } | |||
|
|
|||
| // Increase the buffer size if needed. | |||
| if (nbytes > buffer_->size() - buffer_pos_) { | |||
| if (nbytes > buffer_size_ - buffer_pos_) { | |||
There was a problem hiding this comment.
Should the DCHECK below be changed as well?
There was a problem hiding this comment.
I'll resume buffer_->size() but I think they're same in BufferedBase
| Status SetBufferSize(int64_t new_buffer_size) { | ||
| if (new_buffer_size <= 0) { | ||
| return Status::Invalid("Buffer size should be positive"); | ||
| } | ||
| if ((buffer_pos_ + bytes_buffered_) >= new_buffer_size) { | ||
| return Status::Invalid("Cannot shrink read buffer if buffered data remains"); | ||
| return Status::Invalid( |
There was a problem hiding this comment.
Can this actually occur using the public APIs? If yes, can we add a test?
There was a problem hiding this comment.
Yes actually Peek test tests this. Previously, PeekPastBufferedBytes will raise error here.
Co-authored-by: Antoine Pitrou <[email protected]>
|
By the way, is this rebased on git main already? |
Previously not but rebased now |
pitrou
changed the title
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 7d5a818. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
See #43949
The problem is
PeekandReadboth callsSetBufferSize, however:Readimplicit says that, whenSetBufferSizeor read, the previous buffer is not being required. In this scenerio,bytes_buffered_is always 0, sinceReadwill consume all data. Andnew_buffer_size == std::min(new_buffer_size, (raw_read_bound_ - raw_read_total_))Peekstill requires the buffer-size here. So,bytes_buffered_might not 0. WhenPeek, thenew_buffer_sizewould less than expected, which shrinks the bufferWhat changes are included in this PR?
Update the Logic of
SetBufferSizebytes_buffered_ == 0,SetBufferSizecan discard the current bufferSetBufferSizeshould resize minimal tobuffer_size_ + (raw_read_bound_ - raw_read_total_), since it should considering the current bufferAre these changes tested?
Yes
Are there any user-facing changes?
Bugfix