Seeking from Current(0) return current stream position. #70577
Conversation
The stream_position convenience function is implemented with seek(SeekFrom::Current(0)) and when used with a BufReader this causes the internal buffer to be dumped every time the stream position is queried, which negates many of the benefits when using BufReader with high latency data streams.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Thanks for your contribution :) I think it might be better to overwrite the impl<R: Seek> Seek for BufReader<R> {
fn seek(&mut self, pos: SeekFrom) -> io::Result<u64> {
// ...
}
fn stream_positon(&mut self) -> io::Result<u64> {
Ok(self.pos as u64)
}
} |
There was a problem hiding this comment.
Hi there and thanks for your PR!
You are right, we should certainly improve stream_position. However, I don't think your changes are the way to go, for several reasons. Making sure that this new struct field is always up to date adds maintenance burden, for example. I also think the patch as is is still broken in several places (CI is not green, for example).
The easier and better solution is what @Luro02 suggested. However, as suggested, it is not correct. self.pos does unfortunately not refer to the position in the underlying stream, but is simply the start of the "useful" data in buf. The actual useful buffer is self.buf[self.pos..self.cap], to be precise.
I believe we can obtain the actual stream position like this:
fn stream_positon(&mut self) -> io::Result<u64> {
self.inner.stream_position().map(|inner_pos| inner_pos - self.buffer().len())
}
LukasKalbertodt
added
S-waiting-on-author
|
@maxburke Ping from triage. Could you address the review comments above? Thanks. |
joelpalmer
added
S-waiting-on-author
|
Ping from Triage: Closing due to inactivity. Please re-open with updated. Thanks for the PR! @maxburke |
|
FYI: I tried to apply the implementation suggestions by @Luro02 and @LukasKalbertodt in PR #74366 and added some test case. |
…rtodt Implement Seek::stream_position() for BufReader Optimization over `BufReader::seek()` for getting the current position without flushing the internal buffer. Related to rust-lang#31100. Based on the code in rust-lang#70577.
The stream_position convenience function is implemented with
seek(SeekFrom::Current(0)) and when used with a BufReader this causes
the internal buffer to be dumped every time the stream position is
queried, which negates many of the benefits when using BufReader with
high latency data streams.
See comments in this thread: #31100