From c3fb9bbf08bd3510ab0c31fabab5cb410058883e Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 13 Feb 2025 16:16:33 +0200 Subject: [PATCH 1/4] chore: Use `EnumSet` instead of a Boolean `EnumMap` For `PacketNumberSpaceSet`. May be faster? --- Cargo.lock | 1 + Cargo.toml | 1 + neqo-http3/Cargo.toml | 2 +- neqo-transport/Cargo.toml | 1 + neqo-transport/src/recovery/mod.rs | 13 ++-- neqo-transport/src/tracking.rs | 109 +++++++---------------------- 6 files changed, 35 insertions(+), 92 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e887d9b3b..2e7ac45ee9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -899,6 +899,7 @@ version = "0.12.2" dependencies = [ "criterion", "enum-map", + "enumset", "indexmap", "log", "mtu", diff --git a/Cargo.toml b/Cargo.toml index bb46298543..3cf5c3f03f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,7 @@ rust-version = "1.76.0" [workspace.dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 enum-map = { version = "2.7", default-features = false } +enumset = { version = "1.1", default-features = false } log = { version = "0.4", default-features = false } qlog = { version = "0.13", default-features = false } quinn-udp = { version = "0.5.6", default-features = false, features = ["direct-log", "fast-apple-datapath"] } diff --git a/neqo-http3/Cargo.toml b/neqo-http3/Cargo.toml index ee09eb7729..f640ee0fa6 100644 --- a/neqo-http3/Cargo.toml +++ b/neqo-http3/Cargo.toml @@ -17,7 +17,7 @@ workspace = true [dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 -enumset = { version = "1.1", default-features = false } +enumset = { workspace = true } log = { workspace = true } neqo-common = { path = "./../neqo-common" } neqo-crypto = { path = "./../neqo-crypto" } diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index a3013d6024..b35d9c2503 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -18,6 +18,7 @@ workspace = true [dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 enum-map = { workspace = true } +enumset = { workspace = true } indexmap = { version = "2.2", default-features = false } # See https://github.com/mozilla/neqo/issues/1858 log = { workspace = true } neqo-common = { path = "../neqo-common" } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index e7af86a7b3..1a8914d995 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -21,6 +21,7 @@ use std::{ }; use enum_map::{enum_map, EnumMap}; +use enumset::enum_set; use neqo_common::{qdebug, qinfo, qlog::NeqoQlog, qtrace, qwarn}; pub use sent::SentPacket; use sent::SentPackets; @@ -93,7 +94,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]); + debug_assert!(probe.contains(pn_space)); Self { limit: mtu, pto: Some(pn_space), @@ -107,7 +108,7 @@ impl SendProfile { /// that has the PTO timer armed. #[must_use] pub fn should_probe(&self, space: PacketNumberSpace) -> bool { - self.probe[space] + self.probe.contains(space) } /// Determine whether an ACK-only packet should be sent for the given packet @@ -443,7 +444,7 @@ impl PtoState { } pub fn new(space: PacketNumberSpace, probe: PacketNumberSpaceSet) -> Self { - debug_assert!(probe[space]); + debug_assert!(probe.contains(space)); Self { space, count: 1, @@ -453,7 +454,7 @@ impl PtoState { } pub fn pto(&mut self, space: PacketNumberSpace, probe: PacketNumberSpaceSet) { - debug_assert!(probe[space]); + debug_assert!(probe.contains(space)); self.space = space; self.count += 1; self.packets = Self::pto_packet_count(space); @@ -687,7 +688,7 @@ impl LossRecovery { // So maybe fire a PTO. if let Some(pto) = self.pto_time(rtt, PacketNumberSpace::ApplicationData) { if pto < now { - let probes = PacketNumberSpaceSet::from(&[PacketNumberSpace::ApplicationData]); + let probes = enum_set!(PacketNumberSpace::ApplicationData); self.fire_pto(PacketNumberSpace::ApplicationData, probes, now); } } @@ -826,7 +827,7 @@ impl LossRecovery { let mut allow_probes = PacketNumberSpaceSet::default(); for pn_space in PacketNumberSpace::iter() { if let Some(t) = self.pto_time(rtt, *pn_space) { - allow_probes[*pn_space] = true; + allow_probes.insert(*pn_space); if t <= now { qdebug!("[{self}] PTO timer fired for {pn_space}"); if let Some(space) = self.spaces.get_mut(*pn_space) { diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 6a36d8d192..871acfd3ee 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -9,11 +9,11 @@ use std::{ cmp::min, collections::VecDeque, - ops::{Index, IndexMut}, time::{Duration, Instant}, }; use enum_map::{enum_map, Enum, EnumMap}; +use enumset::{EnumSet, EnumSetType}; use neqo_common::{qdebug, qinfo, qtrace, qwarn, IpTosEcn}; use neqo_crypto::Epoch; @@ -26,7 +26,7 @@ use crate::{ Error, Res, }; -#[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq, Enum)] +#[derive(Debug, PartialOrd, Ord, EnumSetType, Enum)] pub enum PacketNumberSpace { Initial, Handshake, @@ -76,63 +76,7 @@ impl From for PacketNumberSpace { } } -#[derive(Clone, Copy, Default)] -pub struct PacketNumberSpaceSet { - spaces: EnumMap, -} - -impl PacketNumberSpaceSet { - pub fn all() -> Self { - Self { - spaces: enum_map! { - PacketNumberSpace::Initial => true, - PacketNumberSpace::Handshake => true, - PacketNumberSpace::ApplicationData => true, - }, - } - } -} - -impl Index for PacketNumberSpaceSet { - type Output = bool; - - fn index(&self, index: PacketNumberSpace) -> &Self::Output { - &self.spaces[index] - } -} - -impl IndexMut for PacketNumberSpaceSet { - fn index_mut(&mut self, index: PacketNumberSpace) -> &mut Self::Output { - &mut self.spaces[index] - } -} - -impl> From for PacketNumberSpaceSet { - fn from(spaces: T) -> Self { - let mut v = Self::default(); - for sp in spaces.as_ref() { - v[*sp] = true; - } - v - } -} - -impl std::fmt::Debug for PacketNumberSpaceSet { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - let mut first = true; - f.write_str("(")?; - for sp in PacketNumberSpace::iter() { - if self[*sp] { - if !first { - f.write_str("+")?; - first = false; - } - std::fmt::Display::fmt(sp, f)?; - } - } - f.write_str(")") - } -} +pub type PacketNumberSpaceSet = EnumSet; impl std::fmt::Display for PacketNumberSpace { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { @@ -685,6 +629,7 @@ impl Default for AckTracker { mod tests { use std::collections::HashSet; + use enumset::enum_set; use neqo_common::Encoder; use test_fixture::now; @@ -1127,43 +1072,37 @@ mod tests { #[test] fn pnspaceset_default() { let set = PacketNumberSpaceSet::default(); - assert!(!set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); + assert!(!set.contains(PacketNumberSpace::Initial)); + assert!(!set.contains(PacketNumberSpace::Handshake)); + assert!(!set.contains(PacketNumberSpace::ApplicationData)); } #[test] fn pnspaceset_from() { - let set = PacketNumberSpaceSet::from(&[PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); + let set = enum_set!(PacketNumberSpace::Initial); + assert!(set.contains(PacketNumberSpace::Initial)); + assert!(!set.contains(PacketNumberSpace::Handshake)); + assert!(!set.contains(PacketNumberSpace::ApplicationData)); - let set = - PacketNumberSpaceSet::from(&[PacketNumberSpace::Handshake, PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); + let set = enum_set!(PacketNumberSpace::Handshake | PacketNumberSpace::Initial); + assert!(set.contains(PacketNumberSpace::Initial)); + assert!(set.contains(PacketNumberSpace::Handshake)); + assert!(!set.contains(PacketNumberSpace::ApplicationData)); - let set = PacketNumberSpaceSet::from(&[ - PacketNumberSpace::ApplicationData, - PacketNumberSpace::ApplicationData, - ]); - assert!(!set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(set[PacketNumberSpace::ApplicationData]); + let set = + enum_set!(PacketNumberSpace::ApplicationData | PacketNumberSpace::ApplicationData); + assert!(!set.contains(PacketNumberSpace::Initial)); + assert!(!set.contains(PacketNumberSpace::Handshake)); + assert!(set.contains(PacketNumberSpace::ApplicationData)); } #[test] fn pnspaceset_copy() { - let set = PacketNumberSpaceSet::from(&[ - PacketNumberSpace::Handshake, - PacketNumberSpace::ApplicationData, - ]); + let set = enum_set!(PacketNumberSpace::Handshake | PacketNumberSpace::ApplicationData); let copy = set; - assert!(!copy[PacketNumberSpace::Initial]); - assert!(copy[PacketNumberSpace::Handshake]); - assert!(copy[PacketNumberSpace::ApplicationData]); + assert!(!copy.contains(PacketNumberSpace::Initial)); + assert!(copy.contains(PacketNumberSpace::Handshake)); + assert!(copy.contains(PacketNumberSpace::ApplicationData)); } #[test] From b27d9e5c42d504d58eeb621e8e9f851741294582 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 14 Feb 2025 08:40:37 +0200 Subject: [PATCH 2/4] Remove tests as suggested by @martinthomson --- neqo-transport/src/tracking.rs | 41 ++-------------------------------- 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 871acfd3ee..5cf2603324 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -629,13 +629,12 @@ impl Default for AckTracker { mod tests { use std::collections::HashSet; - use enumset::enum_set; use neqo_common::Encoder; use test_fixture::now; use super::{ - AckTracker, Duration, Instant, PacketNumberSpace, PacketNumberSpaceSet, RecoveryToken, - RecvdPackets, MAX_TRACKED_RANGES, + AckTracker, Duration, Instant, PacketNumberSpace, Recovery Token, RecvdPackets, + MAX_TRACKED_RANGES, }; use crate::{ frame::Frame, @@ -1069,42 +1068,6 @@ mod tests { ); } - #[test] - fn pnspaceset_default() { - let set = PacketNumberSpaceSet::default(); - assert!(!set.contains(PacketNumberSpace::Initial)); - assert!(!set.contains(PacketNumberSpace::Handshake)); - assert!(!set.contains(PacketNumberSpace::ApplicationData)); - } - - #[test] - fn pnspaceset_from() { - let set = enum_set!(PacketNumberSpace::Initial); - assert!(set.contains(PacketNumberSpace::Initial)); - assert!(!set.contains(PacketNumberSpace::Handshake)); - assert!(!set.contains(PacketNumberSpace::ApplicationData)); - - let set = enum_set!(PacketNumberSpace::Handshake | PacketNumberSpace::Initial); - assert!(set.contains(PacketNumberSpace::Initial)); - assert!(set.contains(PacketNumberSpace::Handshake)); - assert!(!set.contains(PacketNumberSpace::ApplicationData)); - - let set = - enum_set!(PacketNumberSpace::ApplicationData | PacketNumberSpace::ApplicationData); - assert!(!set.contains(PacketNumberSpace::Initial)); - assert!(!set.contains(PacketNumberSpace::Handshake)); - assert!(set.contains(PacketNumberSpace::ApplicationData)); - } - - #[test] - fn pnspaceset_copy() { - let set = enum_set!(PacketNumberSpace::Handshake | PacketNumberSpace::ApplicationData); - let copy = set; - assert!(!copy.contains(PacketNumberSpace::Initial)); - assert!(copy.contains(PacketNumberSpace::Handshake)); - assert!(copy.contains(PacketNumberSpace::ApplicationData)); - } - #[test] fn from_packet_type() { assert_eq!( From cb36878f3ff55df7070569a478024e6aa3365d5e Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 14 Feb 2025 08:43:03 +0200 Subject: [PATCH 3/4] Fix --- neqo-transport/src/tracking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 5cf2603324..d0716de8b9 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -633,7 +633,7 @@ mod tests { use test_fixture::now; use super::{ - AckTracker, Duration, Instant, PacketNumberSpace, Recovery Token, RecvdPackets, + AckTracker, Duration, Instant, PacketNumberSpace, RecoveryToken, RecvdPackets, MAX_TRACKED_RANGES, }; use crate::{ From 8830fd4a18df4ee859717db0504856aa01d54c15 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 15 Feb 2025 08:02:48 -0800 Subject: [PATCH 4/4] clippy --- neqo-transport/src/tracking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index cffbfb38a9..e92744ac29 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -16,7 +16,7 @@ use enum_map::{enum_map, Enum, EnumMap}; use enumset::{EnumSet, EnumSetType}; use neqo_common::{qdebug, qinfo, qtrace, qwarn, IpTosEcn}; use neqo_crypto::Epoch; -use strum::{EnumIter, IntoEnumIterator as _}; +use strum::EnumIter; use crate::{ ecn,