proto: Replace write_source with WriteGuard<'_> #2242
Draft
+56
−234
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit removes the ByteSource trait and the write_source method.
Instead, a new method is introduced as such:
fn SendStream::write_guard(&mut self) ->
Result<WriteGuard<'_>, WriteError>
The resultant WriteGuard contains the limit of how many bytes could be
written now, as well as borrows of the fields necessary to perform those
writes. The write and write_chunks methods are written to more simply
use this.
djc
reviewed
May 13, 2025
At the |
Ralith
reviewed
May 14, 2025
Comment on lines
+391
to
+394
| stream: &'a mut Send, | ||
| data_sent: &'a mut u64, | ||
| unacked_data: &'a mut u64, | ||
| pending: &'a mut PendingStreamsQueue, |
There was a problem hiding this comment.
Should these be a single reference to a helper struct? That might also be easier to adapt to the quinn layer.
| @@ -367,6 +385,28 @@ impl<'a> SendStream<'a> { | |||
| } | |||
| } | |||
|
|
|||
| struct WriteGuard<'a> { | |||
There was a problem hiding this comment.
bikeshed: WriteBuilder? "Guard" usually means "cleans something up on Drop".
There was a problem hiding this comment.
Maybe StreamWriter? Builder invokes builder pattern for construction to me.
| } | ||
|
|
||
| fn write_source<B: BytesSource>(&mut self, source: &mut B) -> Result<Written, WriteError> { | ||
| fn write_guard(&mut self) -> Result<WriteGuard, WriteError> { |
There was a problem hiding this comment.
bikeshed: build_write, or maybe begin_write.
| Ok(self.write_source(&mut ByteSlice::from_slice(data))?.bytes) | ||
| let mut guard = self.write_guard()?; | ||
| let written = data.len().min(guard.limit); | ||
| guard.write(data[..written].to_vec().into()); |
There was a problem hiding this comment.
Suggested change
| guard.write(data[..written].to_vec().into()); | |
| guard.write(Bytes::copy_from_slice(&data[..written])); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is competing and mutually exclusive with my #2230 PR. In the course of developing 2230 I had the idea that this approach could be tried instead. The impact on LOC is similar (this one is slightly better), and this is simpler in some ways (avoids inversion of control). I'm currently creating this as a draft because I haven't thought about it for very long but you're welcome to look at it and see if you think this is better than 2230?
I did some looking and it looks like the
write_sourceAPI was introduced in #1013. The main aim of that PR seemed to be simply to introduce the ability to directly useBytesin general. This made me wonder why this wasn't done by adding a method that just wrote oneBytesat a time. From looking at the code, it looks like the main penalty to do this would be duplicated hash table look-ups for the stream. Caching the&mut, as this PR does, avoids that.This commit removes the ByteSource trait and the write_source method. Instead, a new method is introduced as such:
fn SendStream::write_guard(&mut self) -> Result<WriteGuard<'_>, WriteError>
The resultant WriteGuard contains the limit of how many bytes could be written now, as well as borrows of the fields necessary to perform those writes. The write and write_chunks methods are written to more simply use this.