refactor: Introduce a TransmitBuf for poll_transmit

[flub]

Hi, I've been looking around in poll_receive recently and started wondering if it could be simplified. This is my first attempt at a step in this direction, it pulls together the state about datagrams and their boundaries in the write buffer. I think this helps understand the interactions between a bunch of state changes that were previously spread across many lines. See the commit message for a detailed description.

If the general direction is considered useful I think I'd like to continue this further. The next thing I'd probably look at is the buffer management for a packet. I expect a lot of places don't need to know much about the full transmit buffer they're writing into but rather need to know the (start, len, end) space they can write their frames. Probably reducing the need to do everything with offsets from the transmit buffer.

Anyway, my aim is the make things clearer and easier to manage. Let's first see if this is step in that direction and then see what the future steps turn out to be. I think any change like this should be able to stand on its own rather than rely on a future change, because the future is always uncertain and might never happen :)

[djc]

Initial feedback from skimming:

• I like the direction!
• In order to make it easy to review this, I'd want the change to be split into smaller commits. I'd suggest by isolating all the state (fields) into TransmitBuf without doing anything else in the first commit, then add the trait(s) in a separate commit and start moving over methods/adding abstractions.
• The BufMut + BufOffset bounds like repetitive, I guess BufOffset could have BufMut as a supertrait probably?
• (Nit: let's start moving towards using impl Trait instead of T: Trait in argument position.)

[flub]

I've attempted to split this up, hopefully the commits are more or less what you had in mind. Still makes the same change essentially.

[djc]

LGTM, did a more detailed pass although I have not completely reviewed all the comments and code changes.

Despite your preferences, please stick to single spaces after periods and US English in this project. 👍

[djc]

I guess? I don't love the LoC overhead this relative to the benefit this provide over an implicit/documentation contract that callers shouldn't be mutating TransmitBuf's fields directly. It is a private API, of course, which we can trivially change over time.

[flub]

That's essentially a comment on this entire commit. I do feel fairly strongly that the encapsulation matters, in Rust this means you have to write accessors and that's fine for me. I don't think a few more lines of code are any overhead, it is even kind of nice you get to write a clear doc comment for it.

The encapsulation is a large part of this PR: the intricate logic of how all these fields relate to each other and the invariants that need to be upheld are now entirely encoded in the 15 lines of new_datagram_inner. And forcing non-mut access to the fields makes it easy to audit this.

[djc]

As suggested, can we make BufLen: BufMut?

[flub]

I chose not to do so because I don't think BufLen is that nice a trait. It is more of a plug to bridge the use of TransmitBuf and the existing concept of passing around a buffer with a bunch of offsets describing it. I hope that later it can be removed again, but also added this to limit the scope of this refactor.

And finally I think the naming would be much less clear, right now places get BufMut + BufLen and you immediately have very clear expectations. If it were to be a supertrait it'd be something like WriteBuffer, BufMutWithLen or I don't know. Naming it would become much trickier.

I do appreciate that this could become unwieldy as more traits are added. Though all the uses for now are just two traits, which I don't think is problematic yet.

[gretchenfrage]

Thank you for working on this! I think this should be considered to close #2121

[gretchenfrage]

Thanks for your patience--I have ambitions of trying to take a closer look at this next weekend.

[gretchenfrage]

Yes, I think this is a good direction! Some thoughts so far...

Naming

First a minor thing: Perhaps this should be called TransmitBuilder rather than TransmitBuf? That way it matches PacketBuilder, and I feel that more accurately represents that that is tracking more complex state than just a byte sequence.

impl BufMut

With regard to this:

unsafe impl BufMut for TransmitBuf<'_> {
    fn remaining_mut(&self) -> usize {
        self.buf.remaining_mut()
    }

    unsafe fn advance_mut(&mut self, cnt: usize) {
        self.buf.advance_mut(cnt);
    }

    fn chunk_mut(&mut self) -> &mut bytes::buf::UninitSlice {
        self.buf.chunk_mut()
    }
}

I'm not sure that I like that. It adds an unsafe statement, as well as a handful of LOC, for the sake of being able to write buf instead of buf.buf in perhaps a dozen places. Plus, it's a slightly surprising thing to keep in mind.

trait BufLen

You're introducing this new trait for the sake of being able to pass around &mut (impl BufMut + BufLen) instead of &mut TransmitBuf. I understand that your motivation for this is to abstract things, but in this circumstance I think that directly passing around &mut TransmitBuf or &mut Vec<u8> to various functions would be a net-win for it being simple to think about.

This is related to my comment about impl BufMut, as creating a version of the "Do not pass buffer around as a Vec" commit which avoids relying on this trait would be easier if we're also not trying to make our code leverage an impl BufMut for TransmitBuf.

Getter methods

I agree with djc here--I'm pretty hesitant about having these getter methods on TransmitBuf: segment_size, num_datagrams, max_datagrams, datagram_start_offset, and datagram_max_offset.

These variables aren't really getting abstracted away if the users of TransmitBuf still have to know which of them exist and care about their specific values. That leaves the only remaining motivation of using getters like this as making sure we don't accidentally mutate them wrongly from outside the module. But it's pretty easy to check where we're writing to these variables and I don't think making these variables read-only from outside this module really gets at the bottleneck of us being able to make the code correct. And it has to be weighed against the cons of increased LOC, and us remembering which ones of these functions are simply reading a field versus which ones are doing something more transformative.

I was experimenting with an alternative way of trying to abstract some of these things, and I noticed that the num_datagrams value is used in similar ways in a few places repeatedly. So I tried to see if I could factor some of them away into more transformative methods on TransmitBuf until the num_datagrams value no longer needed to be accessed outside of TransmitBuf at all and I came up with this:

Diff

diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs
index 3a39a433..62c42e73 100644
--- a/quinn-proto/src/connection/mod.rs
+++ b/quinn-proto/src/connection/mod.rs
@@ -562,7 +562,7 @@ impl Connection {
                 // We need to send 1 more datagram and extend the buffer for that.
 
                 // Is 1 more datagram allowed?
-                if buf.num_datagrams() >= buf.max_datagrams() {
+                if !buf.more_datagrams_allowed() {
                     // No more datagrams allowed
                     break;
                 }
@@ -573,9 +573,7 @@ impl Connection {
                 // for starting another datagram. If there is any anti-amplification
                 // budget left, we always allow a full MTU to be sent
                 // (see https://github.com/quinn-rs/quinn/issues/1082)
-                if self.path.anti_amplification_blocked(
-                    (buf.segment_size() * buf.num_datagrams()) as u64 + 1,
-                ) {
+                if self.path.anti_amplification_blocked(buf.bytes_to_send()) {
                     trace!("blocked by anti-amplification");
                     break;
                 }
@@ -625,7 +623,7 @@ impl Connection {
                         builder.pad_to(MIN_INITIAL_SIZE);
                     }
 
-                    if buf.num_datagrams() > 1 {
+                    if !buf.first_datagram() {
                         // If too many padding bytes would be required to continue the GSO batch
                         // after this packet, end the GSO batch here. Ensures that fixed-size frames
                         // with heterogeneous sizes (e.g. application datagrams) won't inadvertently
@@ -661,7 +659,7 @@ impl Connection {
 
                     builder.finish_and_track(now, self, sent_frames.take(), &mut buf);
 
-                    if buf.num_datagrams() == 1 {
+                    if buf.first_datagram() {
                         buf.clip_datagram_size();
                         if space_id == SpaceId::Data {
                             // Now that we know the size of the first datagram, check
@@ -811,7 +809,7 @@ impl Connection {
 
             // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that path
             // validation can occur while the link is saturated.
-            if space_id == SpaceId::Data && buf.num_datagrams() == 1 {
+            if space_id == SpaceId::Data && buf.first_datagram() {
                 if let Some((token, remote)) = self.path_responses.pop_off_path(self.path.remote) {
                     // `unwrap` guaranteed to succeed because `builder_storage` was populated just
                     // above.
@@ -900,7 +898,6 @@ impl Connection {
                 .mtud
                 .poll_transmit(now, self.packet_number_filter.peek(&self.spaces[space_id]))?;
 
-            debug_assert_eq!(buf.num_datagrams(), 0);
             buf.start_new_datagram_with_size(probe_size as usize);
 
             debug_assert_eq!(buf.datagram_start_offset(), 0);
@@ -933,16 +930,7 @@ impl Connection {
             return None;
         }
 
-        trace!(
-            "sending {} bytes in {} datagrams",
-            buf.len(),
-            buf.num_datagrams()
-        );
-        self.path.total_sent = self.path.total_sent.saturating_add(buf.len() as u64);
-
-        self.stats
-            .udp_tx
-            .on_sent(buf.num_datagrams() as u64, buf.len());
+        buf.update_stats(&mut self.path.total_sent, &mut self.stats.udp_tx);
 
         Some(Transmit {
             destination: self.path.remote,
@@ -952,9 +940,10 @@ impl Connection {
             } else {
                 None
             },
-            segment_size: match buf.num_datagrams() {
-                1 => None,
-                _ => Some(buf.segment_size()),
+            segment_size: if buf.first_datagram() {
+                None
+            } else {
+                Some(buf.segment_size())
             },
             src_ip: self.local_ip,
         })
diff --git a/quinn-proto/src/connection/transmit_buf.rs b/quinn-proto/src/connection/transmit_buf.rs
index 61d3217c..71383945 100644
--- a/quinn-proto/src/connection/transmit_buf.rs
+++ b/quinn-proto/src/connection/transmit_buf.rs
@@ -1,4 +1,7 @@
 use bytes::BufMut;
+use tracing::trace;
+
+use crate::{FrameStats, UdpStats};
 
 use super::BufLen;
 
@@ -151,16 +154,27 @@ impl<'a> TransmitBuf<'a> {
         self.segment_size
     }
 
-    /// Returns the number of datagrams written into the buffer
-    ///
-    /// The last datagram is not necessarily finished yet.
-    pub(super) fn num_datagrams(&self) -> usize {
-        self.num_datagrams
+    pub(super) fn more_datagrams_allowed(&self) -> bool {
+        self.num_datagrams < self.max_datagrams
+    }
+
+    pub(super) fn bytes_to_send(&self) -> u64 {
+        (self.segment_size * self.num_datagrams) as u64 + 1
     }
 
-    /// Returns the maximum number of datagrams allowed to be written into the buffer
-    pub(super) fn max_datagrams(&self) -> usize {
-        self.max_datagrams
+    pub(super) fn first_datagram(&self) -> bool {
+        self.num_datagrams == 1
+    }
+
+    pub(super) fn update_stats(&self, path_total_sent: &mut u64, udp_tx: &mut UdpStats) {
+        trace!(
+            "sending {} bytes in {} datagrams",
+            self.buf.len(),
+            self.num_datagrams,
+        );
+        *path_total_sent = path_total_sent.saturating_add(self.buf.len() as u64);
+
+        udp_tx.on_sent(self.num_datagrams as u64, self.buf.len());
     }
 
     /// Returns the start offset of the current datagram in the buffer

Bear in mind that there might be some flaws in that still at this point, but I think my intent should be clear. No need to incorporate this actual concept into this PR, but I think this approach to abstraction has certain advantages--it more successfully offloads conceptual complexity from the overly large poll_transmit method by extracting inherently meaningful sub-units of its logic into methods more_datagrams_allowed, bytes_to_send, first_datagram and update_stats--and at the end of this, the number of usages of num_datagrams and max_datagrams outside of transmit_buf.rs has dropped to zero and so we can just make them private / delete the getters as a reward.

Similarly, I'm inclined to leaving TransmitBuf.buf as pub(super) and thus removing is_empty, len, and as_mut_slice.

[flub]

Yes, I think this is a good direction! Some thoughts so far...

Naming

First a minor thing: Perhaps this should be called TransmitBuilder rather than TransmitBuf? That way it matches PacketBuilder, and I feel that more accurately represents that that is tracking more complex state than just a byte sequence.

I don't have very strong feelings about the naming, this was really just a working name I thought would change but then just stuck as it did end up making sense. I think both work, but indeed TransmitBuilder would match PacketBuilder (which retains it's name in #2195). The current naming does match the fact it implements BufMut directly. I guess the variable name would also have to change if this was renamed, probably to transmit or so.

impl BufMut

With regard to this:

unsafe impl BufMut for TransmitBuf<'_>

I'm not sure that I like that. It adds an unsafe statement, as well as a handful of LOC, for the sake of being able to write buf instead of buf.buf in perhaps a dozen places. Plus, it's a slightly surprising thing to keep in mind.

What would you think of:

impl TransmitBuf<'_> {
    /// Returns a buffer into which the datagram can be written
    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
        self.buf.limit(self.buf_capacity)
    }
}

This is very similar to what I did in #2195 which has a PacketBuilder::frame_space_mut method which I do rather like how it reads. And it gives protection against writing too many bytes into the buffer. Doing this would indeed alight well with renaming TransmitBuf to TransmitBuilder.

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

trait BufLen

You're introducing this new trait for the sake of being able to pass around &mut (impl BufMut + BufLen) instead of &mut TransmitBuf. I understand that your motivation for this is to abstract things, but in this circumstance I think that directly passing around &mut TransmitBuf or &mut Vec<u8> to various functions would be a net-win for it being simple to think about.

I did this trait for two reasons:

• A number of APIs expect this "I get large buffer with a max write offset (and sometimes need a start offset)" notion, which is what I'm trying to move away from. But to keep the scope of this PR smaller I can't change all of that at once, that would get out of hand. So this BufLen is a stopgap measure.
• I assumed that the functions which took traits rather than the &Vec<u8> wanted to keep doing this. There is some logic to this, because a number of these functions really don't have any business being able to change the state of the datagrams. They only need to write some stuff.

So I think if you receive an explicit &TransmitBuf (or whatever it is called) you need to have some business with datagrams. Only writing some frames is not that.

I think it might be possible to remove BufLen entirely and instead switch to BufMut::remaining_mut for these functions. But:

• I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.
• It would switch around a lot more logic. Using this BufLen keeps the changes to places where this stop-gap measure is needed much simpler and easier to review for correctness.

After #2195 there is only one BufLen remaining. I think it's very reasonable to also remove that and I'd be happy to look at doing that in another PR. Though I'd probably wait to figure out a nice way here until that has all been merged so I know what the final shape of things is and have more of a feel for what everyone agrees on for abstraction style.

Getter methods

I agree with djc here--I'm pretty hesitant about having these getter methods on TransmitBuf: segment_size, num_datagrams, max_datagrams, datagram_start_offset, and datagram_max_offset.

These variables aren't really getting abstracted away if the users of TransmitBuf still have to know which of them exist and care about their specific values. That leaves the only remaining motivation of using getters like this as making sure we don't accidentally mutate them wrongly from outside the module. But it's pretty easy to check where we're writing to these variables and I don't think making these variables read-only from outside this module really gets at the bottleneck of us being able to make the code correct.

This last sentence I am really not that sure about. Abstractions the compiler provides are great at ensuring correctness, having to manually check on each PR if something is being mutated that shouldn't be is very fallible. Having explicit contracts like this will make it clear if that is being changed.

And it has to be weighed against the cons of increased LOC, and us remembering which ones of these functions are simply reading a field versus which ones are doing something more transformative.

I was experimenting with an alternative way of trying to abstract some of these things, and I noticed that the num_datagrams value is used in similar ways in a few places repeatedly. So I tried to see if I could factor some of them away into more transformative methods on TransmitBuf until the num_datagrams value no longer needed to be accessed outside of TransmitBuf at all and I came up with this:

Yes, I agree there may be more scope here. And I was unsure whether to go further or not. For the sake of being more incremental with changes I stopped here for this PR though.

For example I think the bytes_to_send is probably a good one to add. But others I expect to be easier to remove in the future entirely. E.g. datagram_max_offset is very tempting to remove after #2195 where it is only 3 uses remain.

So, from what I understand maybe next steps for this PR could be:

• Rename TransmitBuf to TransmitBuilder
• Add TranmitBuiilder::datagram_mut function
• See which helpers might easily disappear with this, e.g. .len(), .is_empty() etc.

If you think that's helpful I'll give those changes a go.

[flub]

@gretchenfrage I've pushed some new commits. Mainly this finally introduces a buffer abstraction that I've long wanted but resisted creating: the BufSlice (I'm not attached to this name, it could be better). To get access to the datagram being written into you need TransmitBuilder::datagram_mut() (or TransmitBuilder::datagram(), but in return you get a nice access to the buffer.

This means the PacketBuilder now needs to keep track of sizes relative to the datagram buffer, a change I've also long wanted. In turn this means some more of the questionable methods on TransmitBuilder can be removed, which is probably a good gain.

The main reason I was trying to avoid these changes so far was to try and keep the changes smaller. The abstractions are a fair number of lines of code again, but have a straight forward interface.

[flub]

FWIW, TransmitBuilder::datagram_mut() is similar to the approach I already took in #2195 where I used PacketBuilder::frame_space_mut() to access the buffer. Give that model also works well there it gives me confidence this is a good approach.

It does leave a manual BufMut impl around, which I agree is unfortunate due to the unsafe. Though as before it mostly forwards calls to the the underlying buffer implementation. So I don't think this is so bad to have, and Quinn does not seem to have a history of avoiding unsafe.

[flub]

Also note that BufSlice can probably completely remove the need for the BufLen trait from this PR. I'll do that once the BufSlice/DatagramBuffer idea has been accepted and possibly reshaped to reduce churn.

[gretchenfrage]

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc76 and applying this diff seems to work just fine:

diff --git a/quinn-proto/src/connection/transmit_buf.rs b/quinn-proto/src/connection/transmit_buf.rs
index 31fe7e00..b429073d 100644
--- a/quinn-proto/src/connection/transmit_buf.rs
+++ b/quinn-proto/src/connection/transmit_buf.rs
@@ -141,7 +141,7 @@ impl<'a> TransmitBuf<'a> {
     }
 
     /// Returns a buffer into which the current datagram can be written
-    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
+    pub(super) fn datagram_mut<'s>(&'s mut self) -> impl BufMut + super::BufLen + 's {
         self.buf.limit(self.buf_capacity)
     }

[gretchenfrage]

I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.

Can you expand on why not? From looking at it, it actually seems like it might be fine.

BufMut::remaining_mut:

Returns the number of bytes that can be written from the current position until the end of the buffer is reached.

This value is greater than or equal to the length of the slice returned by chunk_mut().

Implementer notes

Implementations of remaining_mut should ensure that the return value does not change unless a call is made to advance_mut or any other function that is documented to change the BufMut’s current position.

Note

remaining_mut may return value smaller than actual available space.

Notable source code bits exemplifying the expected usage of this:

• BufMut::put_slice panics if remaining_mut() < slice.len()
• <Vec<u8> as BufMut>::remaining_mut returns isize::MAX - vec.len()

The comment "This value is greater than or equal to the length of the slice returned by chunk_mut()" clarifies that it's not merely meant as the remaining length of the next chunk, and the put_slice implementation establishes that it's expected to indicate how many more bytes can be written. The Vec implementation seems to further reinforce this understanding. Given this, the comment of "remaining_mut may return value smaller than actual available space" seems to be a pretty low-information note that it can be lower than, say, the extent of the underlying heap allocation or equivalent.

If remaining_mut can be used instead of introducing a new BufLen trait, that sounds quite preferable.

[flub]

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc76 and applying this diff seems to work just fine:

[...] fn datagram_mut<'s>(&'s mut self) -> impl BufMut + BufLen + 's

Oh, interesting. Thanks for pointing this out.

Though in 4b0dcbf I now change that return type to DatagramBuffer. Which, while I failed to remove that comment, I do think can be a rather nice type to return rather than the trait. Because it clearly describes to any user what it is.

[gretchenfrage]

My worry is with the Implementer notes: it says remaining_mut is allowed to change whenever advance_mut is called. But it does not say that it must strictly decrease with the amount that was advanced with. So I imagine it would be legal to re-allocate inside advance_mut so that remaining_mut is now larger then before the call to advance_mut. I'm not sure if this was intentional or not though.

I feel pretty good at this point about just considering that to be less-than-perfect documentation of the bytes crate and assuming that advance_mut is supposed to reduce remaining_mut by exactly cnt. Especially since it's something that's only being used internally.

[gretchenfrage]

What do you think about the BufSlice/DatagramBuffer abstraction though? That is another way of avoiding this, and it's kind of an abstraction that fits nicely to write fixed-sized datagrams (and maybe packets). Though I was worried about the amount of code added for it, now that it's there I do rather like it.

I quite like the DatagramBuffer struct! It seems like a good new and simple abstraction that aligns well with what a lot of this code is expecting. "Growable vector of bytes, with a maximum length, that allocation-wise is actually a suffix of a Vec<u8> that may already have stuff in it." Moreover, if it becomes useful for more things than just datagrams, it could easily be renamed to something more generic.

I don't think it warrants all these traits though, and I think the traits are over-complicated. AFAICT this is the situation that's been created:

flowchart TD
    DatagramBuffer --> BufMut
    DatagramBuffer --> BufSlice
    DatagramBuffer --> BufLen
    V["Vec<u8>"] --> BufMut
    V["Vec<u8>"] --> BufSlice
    V["Vec<u8>"] --> BufLen
    BufSlice --> Deref
    BufSlice --> DerefMut

Loading

And, from checking out bce9653, I can just remove the BufSlice: Deref + DerefMut requirement and it compiles fine:

diff --git a/quinn-proto/src/connection/transmit_builder.rs b/quinn-proto/src/connection/transmit_builder.rs
index dac79621..f79da0ef 100644
--- a/quinn-proto/src/connection/transmit_builder.rs
+++ b/quinn-proto/src/connection/transmit_builder.rs
@@ -203,7 +203,7 @@ impl<'a> TransmitBuilder<'a> {
 ///
 /// The [`Deref`] impl must return the already written bytes as a slice.  [`DerefMut`] must
 /// allow modification of these already written bytes.
-pub(crate) trait BufSlice: BufMut + Deref<Target = [u8]> + DerefMut {
+pub(crate) trait BufSlice: BufMut {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize;

Also the DerefMut implementation overall seems overkill since it's only used in 2 places for minor things, it seems a fair bit simpler to just give it a fn written(&mut self) -> &mut [u8]:

Diff

diff --git a/quinn-proto/src/connection/packet_builder.rs b/quinn-proto/src/connection/packet_builder.rs
index 3052efb2..b1e3484d 100644
--- a/quinn-proto/src/connection/packet_builder.rs
+++ b/quinn-proto/src/connection/packet_builder.rs
@@ -122,7 +122,7 @@ impl PacketBuilder {
         };
         let partial_encode = header.encode(&mut transmits.datagram_mut());
         if conn.peer_params.grease_quic_bit && conn.rng.random() {
-            transmits.datagram_mut()[partial_encode.start] ^= FIXED_BIT;
+            transmits.datagram_mut().written()[partial_encode.start] ^= FIXED_BIT;
         }
 
         let (sample_size, tag_len) = if let Some(ref crypto) = space.crypto {
@@ -255,7 +255,7 @@ impl PacketBuilder {
             .put_bytes(0, packet_crypto.tag_len());
         let encode_start = self.partial_encode.start;
         let mut datagram_buf = transmits.datagram_mut();
-        let packet_buf = &mut datagram_buf[encode_start..];
+        let packet_buf = &mut datagram_buf.written()[encode_start..];
         self.partial_encode.finish(
             packet_buf,
             header_crypto,
diff --git a/quinn-proto/src/connection/transmit_builder.rs b/quinn-proto/src/connection/transmit_builder.rs
index dac79621..968cfaed 100644
--- a/quinn-proto/src/connection/transmit_builder.rs
+++ b/quinn-proto/src/connection/transmit_builder.rs
@@ -1,5 +1,3 @@
-use std::ops::{Deref, DerefMut};
-
 use bytes::BufMut;
 
 use super::BufLen;
@@ -203,7 +201,7 @@ impl<'a> TransmitBuilder<'a> {
 ///
 /// The [`Deref`] impl must return the already written bytes as a slice.  [`DerefMut`] must
 /// allow modification of these already written bytes.
-pub(crate) trait BufSlice: BufMut + Deref<Target = [u8]> + DerefMut {
+pub(crate) trait BufSlice: BufMut {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize;
 
@@ -242,6 +240,10 @@ impl<'a> DatagramBuffer<'a> {
             max_offset,
         }
     }
+
+    pub(crate) fn written(&mut self) -> &mut [u8] {
+        &mut self.buf[self.start_offset..]
+    }
 }
 
 unsafe impl BufMut for DatagramBuffer<'_> {
@@ -258,20 +260,6 @@ unsafe impl BufMut for DatagramBuffer<'_> {
     }
 }
 
-impl Deref for DatagramBuffer<'_> {
-    type Target = [u8];
-
-    fn deref(&self) -> &Self::Target {
-        &self.buf[self.start_offset..]
-    }
-}
-
-impl DerefMut for DatagramBuffer<'_> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.buf[self.start_offset..]
-    }
-}
-
 impl BufSlice for DatagramBuffer<'_> {
     /// Returns the number of already written bytes in the buffer
     fn len(&self) -> usize {

From there I started trying to remove the need to have BufSlice::len / BufLen::len by just being able to take .written().len(), but the way the various traits are integrated together made it more laborious than I'm probably willing to do in the course of writing this comment.

This is really the only reason the BufSlice trait exists for now. It is of course possible to make this w: &mut DatagramBuffer, the cost is a bunch of callers on the receive path would have to be adapted to construct a DatagramBuffer where they currently pass a &mut Vec.

That doesn't seem that hard. But also, could you clarify why that would be necessary on the receive path?

Overall I quite like where the DatagramBuffer abstraction is going. But I think the system of traits surrounding it is over-complicated.

[flub]

9da0da3 continues the DatagramBuffer but removes the BufSlice trait. As you can see in the commit it affects a number of calls on the receive side that pass in a Vec<u8> for a possible response. These calls have been updated, but the DatagramBuffer type is kept internal in quinn-proto, so the public APIs still use Vec<u8> for this.

Regarding the Deref and DerefMut: I think these traits are very natural for this kind of buffer. Removing them now results in 48 failures, because a few more places use the buffer like that now. I now also use Deref<Target = [u8]> for the len implemenation which is needed. If you manually add a DatagramBuffer::len instead of relying on the deref there are only 20 places that use the (mutable) slice access. Still, I think that's enough to show this is a natural API for this so I'm leaning towards keeping this.

f7110c7 removes the BufLen trait again, since no one seems to like this. I did this by essentially moving one of the commits from #2195 here. It uses BufMut::remaining_mut, in a way that even avoids the ambiguity that was discussed above :) (that edge case discussed is avoided thanks to DatagramBuffer being used where that problem occurred). Anyway, I like this result and was only worried about that change making this PR larger than needed.

[flub]

Hi, I would love some feedback on the current state. If this is about right I think the next step would be to tidy up the commits, e.g. there's need to have the TransmitBuf -> TransmitBuilder rename in this and we can probably also avoid the existance of the not-much-loved BufLen in the history. Though to see the incremental changes based on feedback I thought it was better to only add commits for now.

Though if the design needs to evolve more first let me know.

[gretchenfrage]

This is high on my list on things to look at! Just been a bit busy.

[djc]

Feedback from skimming the current state:

• It looks mostly pretty good
• I think the trait hierarchy looks okay
• I don't like the trivial getters on TransmitBuilder, IMO we can just make the fields pub(super) instead
• The start_with_datagram_size() methods and related naming seems a bit unwieldy

I think it's definitely worth getting the commit history in shape here.

[flub]

* I don't like the trivial getters on `TransmitBuilder`, IMO we can just make the fields `pub(super)` instead

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it. I understand that originally this was convenient when first writing Quinn, but also think that it is fine to be more restrictive with the abstractions now.

* The `start_with_datagram_size()` methods and related naming seems a bit unwieldy

Yes, I was exploring this a bit more to see what can be done. This is basically an artifact of how the code was structured before. Having this code abstracted in one place makes it more obvious that this is a bit of a soar point. I think it would totally make sense to further refactor this. E.g. I can easily imagine inverting the api calls from "start new datagram" to
"finish datagram", it would probably remove the need to keep track of variables like num_datagrams as well. But I do think this comes up because inserting this abstraction makes is more easy to recognise this. I would prefer to not to add more to the current PR, I think it already is an improvement on what was there before.

[djc]

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it.

I understand this, but I think we can rely on callers in the limited visibility area to respect this if you just document it. Getters are just not worth it in this case.

[flub]

Hi, just letting you know I'm not forgetting about this PR. Am still waiting for a round of feedback from @gretchenfrage before doing followup changes.

[gretchenfrage]

Thanks for your patience. First off, some things I want to highlight as positives:

• Your clarification of the code comments describing the logic of some of this code is appreciated.
• Renaming to TransmitBuilder seems good.

Some of the comments I attached here I believe are things you fix in later commits, and thus might not be relevant. But yes, as djc mentioned, it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

But yeah I like the direction where this is going.

[gretchenfrage]

Factoring out the cast is not a pure refactor. Consider this snippet, wherein the first debug line succeeds, but the experiences overflow:

#![allow(arithmetic_overflow)]

fn main () {
    const N: u32 = 2_000_000;
    dbg!((N as u64) * (N as u64));
    dbg!((N * N) as u64);
}

Do we have reason to believe that's not a problem here?

[gretchenfrage]

I'm uncertain about whether it's better to have this delegate implementation of BufMut, or to simply pass in &mut buf.buf rather than &mut buf to methods that expect an impl BufMut.

[gretchenfrage]

If there were already a single externally defined trait that could be fully sufficient for replacing &mut Vec<u8>/&mut TransmitBuf with &mut (impl ThatTrait), I would enjoy doing so. But in this case where 1. we are taking on the complexity of defining an additional BufLen trait and its impl blocks 2. we are using compound &mut (impl X + Y) blocks to utilize it and 3. these methods are nevertheless only called with a one implementation, I'm don't really think it's worth it compared to just passing &mut Vec<u8> or &mut TransmitBuf.

[gretchenfrage]

It would be easier to review with our development workflow if you edited your PR history to undo this addition rather than including a commit that adds these lines and a subsequent one that removes them

[gretchenfrage]

+1 on djc's comments on not being fan of the trivial getters for fully internal things

[Ralith]

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

[gretchenfrage]

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

Well, if a project utilizes a "never rebase" workflow, that can make it easier to see what has changed about the PR since last review

[Ralith]

Which is great for the brief period where you can exactly remember what happened last review 😁