Skip to content

Commit 4703853

Browse files
flubflub
committed
Move allocating new datagrams into TransmitBuilder
This moves the logic of updating the buffer for new datagrams into a function on the TransmitBuilder. This centralises all the manipulation of these variables into one logical place, ensuring all the invariants are upheld.
1 parent bee2b1a commit 4703853

File tree

2 files changed

+101
-57
lines changed

2 files changed

+101
-57
lines changed

‎quinn-proto/src/connection/mod.rs‎

Lines changed: 26 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -661,70 +661,42 @@ impl Connection {
661661

662662
builder.finish_and_track(now, self, sent_frames.take(), transmit.buf);
663663

664-
if transmit.num_datagrams == 1 {
665-
// Set the segment size for this GSO batch to the size of the first UDP
666-
// datagram in the batch. Larger data that cannot be fragmented
667-
// (e.g. application datagrams) will be included in a future batch. When
668-
// sending large enough volumes of data for GSO to be useful, we expect
669-
// packet sizes to usually be consistent, e.g. populated by max-size STREAM
670-
// frames or uniformly sized datagrams.
671-
transmit.segment_size = transmit.len();
672-
// Clip the unused capacity out of the buffer so future packets don't
673-
// overrun
674-
transmit.buf_capacity = transmit.len();
675-
676-
// Check whether the data we planned to send will fit in the reduced segment
677-
// size. If not, bail out and leave it for the next GSO batch so we don't
678-
// end up trying to send an empty packet. We can't easily compute the right
679-
// segment size before the original call to `space_can_send`, because at
680-
// that time we haven't determined whether we're going to coalesce with the
681-
// first datagram or potentially pad it to `MIN_INITIAL_SIZE`.
682-
if space_id == SpaceId::Data {
683-
let frame_space_1rtt = transmit
684-
.segment_size
685-
.saturating_sub(self.predict_1rtt_overhead(Some(pn)));
686-
if self.space_can_send(space_id, frame_space_1rtt).is_empty() {
687-
break;
688-
}
664+
if transmit.num_datagrams == 1 && space_id == SpaceId::Data {
665+
// Now that we know the size of the first datagram, check whether
666+
// the data we planned to send will fit in the next segment. If
667+
// not, bails out and leave it for the next GSO batch. We can't
668+
// easily compute the right segment size before the original call to
669+
// `space_can_send`, because at that time we haven't determined
670+
// whether we're going to coalesce with the first datagram or
671+
// potentially pad it to `MIN_INITIAL_SIZE`.
672+
673+
transmit.clip_datagram_size();
674+
675+
let frame_space_1rtt = transmit
676+
.segment_size
677+
.saturating_sub(self.predict_1rtt_overhead(Some(pn)));
678+
if self.space_can_send(space_id, frame_space_1rtt).is_empty() {
679+
break;
689680
}
690681
}
691682
}
692683

693-
// Allocate space for another datagram
694-
let next_datagram_size_limit = match self.spaces[space_id].loss_probes {
695-
0 => transmit.segment_size,
684+
// Start the next datagram
685+
match self.spaces[space_id].loss_probes {
686+
0 => transmit.start_new_datagram(),
696687
_ => {
697688
self.spaces[space_id].loss_probes -= 1;
698689
// Clamp the datagram to at most the minimum MTU to ensure that loss probes
699690
// can get through and enable recovery even if the path MTU has shrank
700691
// unexpectedly.
701-
std::cmp::min(transmit.segment_size, usize::from(INITIAL_MTU))
692+
transmit.start_new_datagram_with_size(std::cmp::min(
693+
usize::from(INITIAL_MTU),
694+
transmit.segment_size,
695+
));
702696
}
703697
};
704-
transmit.buf_capacity += next_datagram_size_limit;
705-
if transmit.buf.capacity() < transmit.buf_capacity {
706-
// We reserve the maximum space for sending `max_datagrams` upfront
707-
// to avoid any reallocations if more datagrams have to be appended later on.
708-
// Benchmarks have shown shown a 5-10% throughput improvement
709-
// compared to continuously resizing the datagram buffer.
710-
// While this will lead to over-allocation for small transmits
711-
// (e.g. purely containing ACKs), modern memory allocators
712-
// (e.g. mimalloc and jemalloc) will pool certain allocation sizes
713-
// and therefore this is still rather efficient.
714-
transmit
715-
.buf
716-
.reserve(transmit.max_datagrams * transmit.segment_size);
717-
}
718-
transmit.num_datagrams += 1;
719698
coalesce = true;
720699
pad_datagram = false;
721-
transmit.datagram_start = transmit.len();
722-
723-
debug_assert_eq!(
724-
transmit.datagram_start % transmit.segment_size,
725-
0,
726-
"datagrams in a GSO batch must be aligned to the segment size"
727-
);
728700
} else {
729701
// We can append/coalesce the next packet into the current
730702
// datagram.
@@ -926,8 +898,8 @@ impl Connection {
926898
.mtud
927899
.poll_transmit(now, self.packet_number_filter.peek(&self.spaces[space_id]))?;
928900

929-
transmit.buf_capacity = probe_size as usize;
930-
transmit.buf.reserve(transmit.buf_capacity);
901+
debug_assert_eq!(transmit.num_datagrams, 0);
902+
transmit.start_new_datagram_with_size(probe_size as usize);
931903

932904
debug_assert_eq!(transmit.datagram_start, 0);
933905
let mut builder = PacketBuilder::new(
@@ -957,7 +929,6 @@ impl Connection {
957929
builder.finish_and_track(now, self, Some(sent_frames), transmit.buf);
958930

959931
self.stats.path.sent_plpmtud_probes += 1;
960-
transmit.num_datagrams = 1;
961932

962933
trace!(?probe_size, "writing MTUD probe");
963934
}
@@ -1013,9 +984,7 @@ impl Connection {
1013984
SpaceId::Data,
1014985
"PATH_CHALLENGE queued without 1-RTT keys"
1015986
);
1016-
buf.buf.reserve(MIN_INITIAL_SIZE as usize);
1017-
1018-
buf.buf_capacity = buf.buf.capacity();
987+
buf.start_new_datagram_with_size(MIN_INITIAL_SIZE as usize);
1019988

1020989
// Use the previous CID to avoid linking the new path with the previous path. We
1021990
// don't bother accounting for possible retirement of that prev_cid because this is

‎quinn-proto/src/connection/transmit_builder.rs‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,81 @@ impl<'a> TransmitBuilder<'a> {
6565
}
6666
}
6767

68+
/// Starts a datagram with a custom datagram size
69+
///
70+
/// This is a specialized version of [`TransmitBuf::start_new_datagram`] which sets the
71+
/// datagram size. Useful for e.g. PATH_CHALLENGE, tail-loss probes or MTU probes.
72+
///
73+
/// After the first datagram you can never increase the segment size. If you decrease
74+
/// the size of a datagram in a batch, it must be the last datagram of the batch.
75+
pub(super) fn start_new_datagram_with_size(&mut self, datagram_size: usize) {
76+
// Only reserve space for this datagram, usually it is the last one in the batch.
77+
let max_capacity_hint = datagram_size;
78+
self.new_datagram_inner(datagram_size, max_capacity_hint)
79+
}
80+
81+
/// Starts a new datagram in the transmit buffer
82+
///
83+
/// If this starts the second datagram the segment size will be set to the size of the
84+
/// first datagram.
85+
///
86+
/// If the underlying buffer does not have enough capacity yet this will allocate enough
87+
/// capacity for all the datagrams allowed in a single batch. Use
88+
/// [`TransmitBuf::start_new_datagram_with_size`] if you know you will need less.
89+
pub(super) fn start_new_datagram(&mut self) {
90+
// We reserve the maximum space for sending `max_datagrams` upfront to avoid any
91+
// reallocations if more datagrams have to be appended later on. Benchmarks have
92+
// shown a 5-10% throughput improvement compared to continuously resizing the
93+
// datagram buffer. While this will lead to over-allocation for small transmits
94+
// (e.g. purely containing ACKs), modern memory allocators (e.g. mimalloc and
95+
// jemalloc) will pool certain allocation sizes and therefore this is still rather
96+
// efficient.
97+
let max_capacity_hint = self.max_datagrams * self.segment_size;
98+
self.new_datagram_inner(self.segment_size, max_capacity_hint)
99+
}
100+
101+
fn new_datagram_inner(&mut self, datagram_size: usize, max_capacity_hint: usize) {
102+
debug_assert!(self.num_datagrams < self.max_datagrams);
103+
if self.num_datagrams == 1 {
104+
// Set the segment size to the size of the first datagram.
105+
self.segment_size = self.buf.len();
106+
}
107+
if self.num_datagrams >= 1 {
108+
debug_assert!(datagram_size <= self.segment_size);
109+
if datagram_size < self.segment_size {
110+
// If this is a GSO batch and this datagram is smaller than the segment
111+
// size, this must be the last datagram in the batch.
112+
self.max_datagrams = self.num_datagrams + 1;
113+
}
114+
}
115+
self.datagram_start = self.buf.len();
116+
debug_assert_eq!(
117+
self.datagram_start % self.segment_size,
118+
0,
119+
"datagrams in a GSO batch must be aligned to the segment size"
120+
);
121+
self.buf_capacity = self.datagram_start + datagram_size;
122+
if self.buf_capacity > self.buf.capacity() {
123+
self.buf
124+
.reserve_exact(max_capacity_hint.saturating_sub(self.buf.capacity()));
125+
}
126+
self.num_datagrams += 1;
127+
}
128+
129+
/// Clips the datagram size to the current size
130+
///
131+
/// Only valid for the first datagram, when the datagram might be smaller than the
132+
/// segment size. Needed before estimating the available space in the next datagram
133+
/// based on [`TransmitBuf::segment_size`].
134+
///
135+
/// Use [`TransmitBuf::start_new_datagram_with_size`] if you need to reduce the size of
136+
/// the last datagram in a batch.
137+
pub(super) fn clip_datagram_size(&mut self) {
138+
debug_assert_eq!(self.num_datagrams, 1);
139+
self.segment_size = self.buf.len();
140+
self.buf_capacity = self.buf.len();
141+
}
142+
68143
/// Returns `true` if the buffer did not have anything written into it
69144
pub(super) fn is_empty(&self) -> bool {
70145
self.len() == 0

0 commit comments

Comments
 (0)