Flub/poll transmit ref #54
Merged
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 TransmitBuf is a wrapper around the buffer in which datagrams are being created. It keeps track of the state required to know the boundaries of the datagrams in the buffer.
This removes the extra arguments from Packetbuilder::new that tell the builder about datagram boundaries in favour of using the state of TransmitBuf directly.
This moves the logic of updating the buffer for new datagrams into a function on the TransmitBuf. This centralises all the manipulation of these variables into one logical place, ensuring all the invariants are upheld.
This helps encapsulation by ensuring that no outside users can mutate these fields. Reducing the amount of logic that needs to be reasoned about.
We want to hide the actual buffer implementation from most of these locations. Currently it is a TransmitBuf but it likely will become something else in the future. So make this generic.
This allows making TransmitBuf::buf private at last. Now all the logic it handles is fully encapsulated.
This re-arranges the loop in poll_transmit to always finish the packet before going to the next iteration. This primarily enables to mutably borrow the TransmitBuf into a packet-specific buffer while the packet is being built. But this is not yet utilised in this commit. It does however remove the need of the mutable builder_storage Option, which makes reasoning over packet building slightly easier. - The logic to know on which packet space to send next, or whether there is no longer anything to send, has been moved to the next_send_space method. - The logic to decide whether to pad a packet before finishing it is moved to the end of the loop. - The logic to check the congestion controller and pacing is kept at the start of the loop. Before a new packet is started. Starting a new datagram also stays there.
Now that the packet is always finished at the end of the loop we no longer need to carry around the mutable SentFrames. Reducing further the number of things to keep track of.
The PacketBuilder::finish_and_track function took a SentFrames argument as an Option. However this was an artifact of the poll_transmit loop tracking it in an Option before, and it not being clear this was always Some when things were going correctly. Now all the callers clearly always pass this in so we can remove the Option.
Now the lifetimes allow for this the PacketBuilder can own the TransmitBuf. This is gives it more control over the buffer into which the packet can be written. This commit itself does nothing interesting with this yet. It merely moves the buffer ownership in a mechanical way. However, this enables future changes to reduce the use of offsets in so many places.
This moves keeping track of the available frame space to the packet builder. The available space is now encoded into the BufMut returned by PacketBuilder::frame_space_mut(). This removes the need for the BufLen trait in many of the places writing frames.
Now that the PacketBuilder::frame_space_mut exists the direct BufMut impl on it can be removed. Nothing external needs to directly write into the packet buffer outside of the frame space.
It no longer needs to keep track of this field because the TransmitBuf already does. Removing duplicate state is nice.
You always need to remember to handle pad_datagram if needed. While really this always happens just before the call to finish_and_track. Instead this can be done in finish_and_track without any logic change, and this helps avoiding mistakes.
flub
force-pushed
the
flub/poll-transmit-ref
branch
from
247a07d to
a0ba958
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 applies the refactors from quinn-rs#2168 and quinn-rs#2195 onto our multipath branch!