From baae4f2ab4f2792f4fc13b9e75608cb57a092ceb Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 26 Nov 2024 09:48:49 +0100 Subject: [PATCH] bench(transport/recovery): benchmark `SentPackets::take_ranges` (#2253) * bench: add SentPackets::take_ranges benchmark * Fix clippy lints for pub mod packets, recovery and sent with feat bench In order to benchmark `SentPackets::take_ranges`, we need to make `packets`, `recovery` and `sent` public modules, feature flagged with `bench`. Public modules have stricter clippy lints. This commit addresses the failing clippy lints. * Trigger benchmark * Update neqo-transport/src/recovery/mod.rs Co-authored-by: Martin Thomson Signed-off-by: Max Inden * Remove useless is_empty --------- Signed-off-by: Max Inden Co-authored-by: Martin Thomson --- neqo-transport/Cargo.toml | 5 +++ neqo-transport/benches/sent_packets.rs | 52 ++++++++++++++++++++++++++ neqo-transport/src/lib.rs | 7 +++- neqo-transport/src/packet/mod.rs | 1 + neqo-transport/src/recovery/mod.rs | 34 ++++++++++++++++- neqo-transport/src/recovery/sent.rs | 18 +++++++++ 6 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 neqo-transport/benches/sent_packets.rs diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index 5a389f2e6b..0a39b52c7f 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -58,3 +58,8 @@ required-features = ["bench"] name = "range_tracker" harness = false required-features = ["bench"] + +[[bench]] +name = "sent_packets" +harness = false +required-features = ["bench"] diff --git a/neqo-transport/benches/sent_packets.rs b/neqo-transport/benches/sent_packets.rs new file mode 100644 index 0000000000..58f384b7de --- /dev/null +++ b/neqo-transport/benches/sent_packets.rs @@ -0,0 +1,52 @@ +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::time::Instant; + +use criterion::{criterion_group, criterion_main, Criterion}; +use neqo_common::IpTosEcn; +use neqo_transport::{ + packet::{PacketNumber, PacketType}, + recovery::sent::{SentPacket, SentPackets}, +}; + +fn sent_packets() -> SentPackets { + let mut pkts = SentPackets::default(); + let now = Instant::now(); + // Simulate high bandwidth-delay-product connection. + for i in 0..2_000u64 { + pkts.track(SentPacket::new( + PacketType::Short, + PacketNumber::from(i), + IpTosEcn::default(), + now, + true, + Vec::new(), + 100, + )); + } + pkts +} + +/// Confirm that taking a small number of ranges from the front of +/// a large span of sent packets is performant. +/// This is the most common pattern when sending at a higher rate. +/// New acknowledgments will include low-numbered packets, +/// while the acknowledgment rate will ensure that most of the +/// outstanding packets remain in flight. +fn take_ranges(c: &mut Criterion) { + c.bench_function("SentPackets::take_ranges", |b| { + b.iter_batched_ref( + sent_packets, + // Take the first 90 packets, minus some gaps. + |pkts| pkts.take_ranges([70..=89, 40..=59, 10..=29]), + criterion::BatchSize::SmallInput, + ); + }); +} + +criterion_group!(benches, take_ranges); +criterion_main!(benches); diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 7b650b7f2e..2507da988a 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -23,14 +23,17 @@ pub mod frame; #[cfg(not(fuzzing))] mod frame; mod pace; -#[cfg(fuzzing)] +#[cfg(any(fuzzing, feature = "bench"))] pub mod packet; -#[cfg(not(fuzzing))] +#[cfg(not(any(fuzzing, feature = "bench")))] mod packet; mod path; mod pmtud; mod qlog; mod quic_datagrams; +#[cfg(feature = "bench")] +pub mod recovery; +#[cfg(not(feature = "bench"))] mod recovery; #[cfg(feature = "bench")] pub mod recv_stream; diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs index 09a4e19d26..8f7ace63f1 100644 --- a/neqo-transport/src/packet/mod.rs +++ b/neqo-transport/src/packet/mod.rs @@ -764,6 +764,7 @@ impl<'a> PublicPacket<'a> { self.version.unwrap_or(0) } + #[allow(clippy::len_without_is_empty)] #[must_use] pub const fn len(&self) -> usize { self.data.len() diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index 5400d631f5..8f89b1cfcd 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -6,6 +6,9 @@ // Tracking of sent packets and detecting their loss. +#[cfg(feature = "bench")] +pub mod sent; +#[cfg(not(feature = "bench"))] mod sent; mod token; @@ -62,6 +65,7 @@ pub struct SendProfile { } impl SendProfile { + #[must_use] pub fn new_limited(limit: usize) -> Self { // When the limit is too low, we only send ACK frames. // Set the limit to `ACK_ONLY_SIZE_LIMIT - 1` to ensure that @@ -74,6 +78,7 @@ impl SendProfile { } } + #[must_use] pub fn new_paced() -> Self { // When pacing, we still allow ACK frames to be sent. Self { @@ -84,6 +89,7 @@ impl SendProfile { } } + #[must_use] pub fn new_pto(pn_space: PacketNumberSpace, mtu: usize, probe: PacketNumberSpaceSet) -> Self { debug_assert!(mtu > ACK_ONLY_SIZE_LIMIT); debug_assert!(probe[pn_space]); @@ -98,6 +104,7 @@ impl SendProfile { /// Whether probing this space is helpful. This isn't necessarily the space /// that caused the timer to pop, but it is helpful to send a PING in a space /// that has the PTO timer armed. + #[must_use] pub fn should_probe(&self, space: PacketNumberSpace) -> bool { self.probe[space] } @@ -106,14 +113,17 @@ impl SendProfile { /// number space. /// Send only ACKs either: when the space available is too small, or when a PTO /// exists for a later packet number space (which should get the most space). + #[must_use] pub fn ack_only(&self, space: PacketNumberSpace) -> bool { self.limit < ACK_ONLY_SIZE_LIMIT || self.pto.is_some_and(|sp| space < sp) } + #[must_use] pub const fn paced(&self) -> bool { self.paced } + #[must_use] pub const fn limit(&self) -> usize { self.limit } @@ -141,6 +151,7 @@ pub struct LossRecoverySpace { } impl LossRecoverySpace { + #[must_use] pub fn new(space: PacketNumberSpace) -> Self { Self { space, @@ -161,6 +172,7 @@ impl LossRecoverySpace { self.first_ooo_time } + #[must_use] pub const fn in_flight_outstanding(&self) -> bool { self.in_flight_outstanding > 0 } @@ -179,6 +191,7 @@ impl LossRecoverySpace { .take(count) } + #[must_use] pub fn pto_base_time(&self) -> Option { if self.in_flight_outstanding() { debug_assert!(self.last_ack_eliciting.is_some()); @@ -220,6 +233,7 @@ impl LossRecoverySpace { /// send a PING frame after 1 PTO. Note that this can't be within a PTO, or /// we would risk setting up a feedback loop; having this many packets /// outstanding can be normal and we don't want to PING too often. + #[must_use] pub fn should_probe(&self, pto: Duration, now: Instant) -> bool { let n_pto = if self.sent_packets.len() >= MAX_OUTSTANDING_UNACK { 1 @@ -376,6 +390,7 @@ impl LossRecoverySpaces { sp.unwrap().remove_ignored() } + #[must_use] pub fn get(&self, space: PacketNumberSpace) -> Option<&LossRecoverySpace> { self.spaces[space].as_ref() } @@ -486,6 +501,7 @@ pub struct LossRecovery { } impl LossRecovery { + #[must_use] pub fn new(stats: StatsCell, fast_pto: u8) -> Self { Self { confirmed_time: None, @@ -497,6 +513,7 @@ impl LossRecovery { } } + #[must_use] pub fn largest_acknowledged_pn(&self, pn_space: PacketNumberSpace) -> Option { self.spaces.get(pn_space).and_then(|sp| sp.largest_acked) } @@ -505,9 +522,14 @@ impl LossRecovery { self.qlog = qlog; } + /// Drop all 0rtt packets. + /// + /// # Panics + /// + /// Panics when the largest acknowledged or `loss_time` is already set. + /// The client should not have received any ACK frames in the + /// application data packet number space when it drops 0-RTT. pub fn drop_0rtt(&mut self, primary_path: &PathRef, now: Instant) -> Vec { - // The largest acknowledged or loss_time should still be unset. - // The client should not have received any ACK frames when it drops 0-RTT. assert!(self .spaces .get(PacketNumberSpace::ApplicationData) @@ -543,6 +565,12 @@ impl LossRecovery { } } + /// Whether to probe the path. + /// + /// # Panics + /// + /// Assumes application data packet number space to be present. + #[must_use] pub fn should_probe(&self, pto: Duration, now: Instant) -> bool { self.spaces .get(PacketNumberSpace::ApplicationData) @@ -574,6 +602,7 @@ impl LossRecovery { /// Returns (acked packets, lost packets) #[allow(clippy::too_many_arguments)] + #[allow(clippy::missing_panics_doc)] pub fn on_ack_received( &mut self, primary_path: &PathRef, @@ -725,6 +754,7 @@ impl LossRecovery { /// Calculate when the next timeout is likely to be. This is the earlier of the loss timer /// and the PTO timer; either or both might be disabled, so this can return `None`. + #[must_use] pub fn next_timeout(&self, path: &Path) -> Option { let rtt = path.rtt(); let loss_time = self.earliest_loss_time(rtt); diff --git a/neqo-transport/src/recovery/sent.rs b/neqo-transport/src/recovery/sent.rs index 8d7f87c476..4b7a7ab6df 100644 --- a/neqo-transport/src/recovery/sent.rs +++ b/neqo-transport/src/recovery/sent.rs @@ -37,6 +37,7 @@ pub struct SentPacket { } impl SentPacket { + #[must_use] pub const fn new( pt: PacketType, pn: PacketNumber, @@ -61,41 +62,50 @@ impl SentPacket { } /// The type of this packet. + #[must_use] pub const fn packet_type(&self) -> PacketType { self.pt } /// The number of the packet. + #[must_use] pub const fn pn(&self) -> PacketNumber { self.pn } /// The ECN mark of the packet. + #[must_use] pub const fn ecn_mark(&self) -> IpTosEcn { self.ecn_mark } /// The time that this packet was sent. + #[must_use] pub const fn time_sent(&self) -> Instant { self.time_sent } /// Returns `true` if the packet will elicit an ACK. + #[must_use] pub const fn ack_eliciting(&self) -> bool { self.ack_eliciting } /// Returns `true` if the packet was sent on the primary path. + #[must_use] pub const fn on_primary_path(&self) -> bool { self.primary_path } /// The length of the packet that was sent. + #[allow(clippy::len_without_is_empty)] + #[must_use] pub const fn len(&self) -> usize { self.len } /// Access the recovery tokens that this holds. + #[must_use] pub fn tokens(&self) -> &[RecoveryToken] { &self.tokens } @@ -113,6 +123,7 @@ impl SentPacket { } /// Whether the packet has been declared lost. + #[must_use] pub const fn lost(&self) -> bool { self.time_declared_lost.is_some() } @@ -123,11 +134,13 @@ impl SentPacket { /// and has not previously been declared lost. /// Note that this should count packets that contain only ACK and PADDING, /// but we don't send PADDING, so we don't track that. + #[must_use] pub const fn cc_outstanding(&self) -> bool { self.ack_eliciting() && self.on_primary_path() && !self.lost() } /// Whether the packet should be tracked as in-flight. + #[must_use] pub const fn cc_in_flight(&self) -> bool { self.ack_eliciting() && self.on_primary_path() } @@ -144,18 +157,21 @@ impl SentPacket { /// Ask whether this tracked packet has been declared lost for long enough /// that it can be expired and no longer tracked. + #[must_use] pub fn expired(&self, now: Instant, expiration_period: Duration) -> bool { self.time_declared_lost .is_some_and(|loss_time| (loss_time + expiration_period) <= now) } /// Whether the packet contents were cleared out after a PTO. + #[must_use] pub const fn pto_fired(&self) -> bool { self.pto } /// On PTO, we need to get the recovery tokens so that we can ensure that /// the frames we sent can be sent again in the PTO packet(s). Do that just once. + #[must_use] pub fn pto(&mut self) -> bool { if self.pto || self.lost() { false @@ -174,6 +190,8 @@ pub struct SentPackets { } impl SentPackets { + #[allow(clippy::len_without_is_empty)] + #[must_use] pub fn len(&self) -> usize { self.packets.len() }