[runtime/network/tokio] add read buffering #2593
Conversation
Add an internal read buffer to the tokio Stream implementation to reduce syscall overhead. Multiple small recv() calls can now be satisfied from the buffer without additional network operations. Key changes: - Add configurable read_buffer_size to Config (default 64KB) - Buffer smaller reads to batch syscalls - For large reads (>= buffer size), bypass buffer and read directly
Deploying monorepo with
|
| Latest commit: |
f479216
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2b4e64a9.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://claude-add-tokio-read-buffer.monorepo-eu0.pages.dev |
|
|
||
| // Time out if we take too long to read | ||
| timeout(self.read_timeout, self.stream.read_exact(buf.as_mut())) | ||
| // First, drain any buffered data |
There was a problem hiding this comment.
Tokio has some built-ins but AFAICT nothing quite right
| // Read at least min_bytes more, up to buffer capacity | ||
| while self.end < target { | ||
| // Compute the remaining time and check if we've timed out | ||
| let remaining_time = deadline.saturating_duration_since(tokio::time::Instant::now()); |
There was a problem hiding this comment.
This is the only real ~gotcha on this PR! We don't want to let an adversary force us to sit in this loop indefinitely by feeding us 1 byte at a time.
| let target = self.end + min_bytes; | ||
|
|
||
| // Read at least min_bytes more, up to buffer capacity | ||
| while self.end < target { |
There was a problem hiding this comment.
What's preventing me from trying to fill_buffer with min_bytes >> self.buffer.len() ? Because it looks like you'll infinite loop in that event (since self.end will always be less than target)?
There was a problem hiding this comment.
(If this should never happen, perhaps document the invariant with an assertion check on the min_bytes input)
There was a problem hiding this comment.
That's a case we check before starting the buffered read.
There was a problem hiding this comment.
Yeah I see where min_bytes is computed on L138 but feels like you should still assert it's not too big within this function, in case another use cases pops up and someone gets it wrong...
| read_timeout: Duration, | ||
| stream: OwnedReadHalf, | ||
| /// Internal buffer for batching reads. | ||
| buffer: Vec<u8>, |
There was a problem hiding this comment.
We should use BytesMut here along with OwnedReadHalf::read_buf, to align with #2558. Would also get rid of the need to manually track start/end.
|
|
||
| let remaining = self.end - self.start; | ||
| if remaining > 0 { | ||
| self.buffer.copy_within(self.start..self.end, 0); |
There was a problem hiding this comment.
I'm not sure we ever need to actually do this. Whenever we need to fill the buffer, there is nothing of interest left (should I think be good to just start at 0 in buffered read).
|
Replaced by #2595 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2593 +/- ##
==========================================
- Coverage 92.63% 92.61% -0.02%
==========================================
Files 353 353
Lines 102482 102577 +95
==========================================
+ Hits 94932 95005 +73
- Misses 7550 7572 +22
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Related: #786
Add an internal read buffer to the tokio Stream implementation to reduce
syscall overhead. Multiple small recv() calls can now be satisfied from
the buffer without additional network operations.
Key changes: