Repository navigation
Bound the bytes returned by a single TlsStream::poll_read - #209
rdettai-sk wants to merge 2 commits into
Conversation
rustls#198 made poll_read drain as many records as fit in the caller's buffer. With a large buffer that turns one poll into an unbounded amount of decryption work. Stop batching once 128 KiB have been copied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Could/should we bound this by time instead of space?
| Poll::Pending => break, | ||
| }; | ||
| let len = Ord::min(data.len(), buf.remaining()); | ||
| let len = Ord::min(data.len(), buf.remaining()).min(MAX_READ_PER_POLL - read); |
There was a problem hiding this comment.
Nit: please use Ord::min() instead of the method variant.
There was a problem hiding this comment.
I don't think this clamping is even worth it 😄
| Poll::Pending => break, | ||
| }; | ||
| let len = Ord::min(data.len(), buf.remaining()); | ||
| let len = Ord::min(data.len(), buf.remaining()).min(MAX_READ_PER_POLL - read); |
Clamping the last chunk saves no decryption: the record is already decrypted by the time it is copied. Overshooting by less than one record keeps the loop identical to the one from rustls#198 apart from its condition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
You're call. This feels simple and easy to test, keeps the batching consistently while providing a safeguard. |
|
Were you going to benchmark this in production? Could you benchmark both? |
|
I can benchmark this in a large scale setup! |
|
Would be nice! |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I didn't get a chance to run the bench on the time bound version (#210) yet. For this PR:
From these results, I would rather run 0.26.4, as the slight hypothetical CPU gain is not really worth the stability loss on the Tokio runtime. Next steps: if I find the time I would try yielding after 64KB and the time bound yield. |
2 participants
Follow-up to #198, as discussed there.
#198 made
TlsStream::poll_readkeep callingpoll_fill_bufuntil the caller'sReadBufis full. That saves a lot of wake-ups. But each extra record costs a decryption, and the batch is only limited by how big the caller's buffer is. Hyper grows its read buffer up to ~400 KiB, and other users oftokio-rustlsmay pass even larger buffers. When many streams share a task, the longer polls add up.This PR caps the batch: once a single
poll_readhas copied at leastMAX_READ_PER_POLL(128 KiB) bytes, it stops pulling records and returnsReadywith what it has. The rest stays buffered in rustls for the next call.common.Choosing the value. 128 KiB is 8 full records. That keeps most of the gain from #198 (8x fewer polls than one record per poll), and it bounds per-poll work to about a third of hyper's largest buffer. I'm happy to change the value or make it configurable if you prefer.
🤖 Generated with Claude Code