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 <[email protected]>
Signed-off-by: Max Inden <[email protected]>

* Remove useless is_empty

---------

Signed-off-by: Max Inden <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>

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"]

neqo-transport/benches/sent_packets.rs

@@ -0,0 +1,52 @@
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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);

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;

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()

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<Instant> {
         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<PacketNumber> {
         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<SentPacket> {
-        // 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<R>(
         &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<Instant> {
         let rtt = path.rtt();
         let loss_time = self.earliest_loss_time(rtt);

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()
     }