From 198c006b0af668f3f89d0aadaa1dc5c451e63850 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 25 Sep 2024 09:46:55 +0200 Subject: [PATCH 1/2] perf(transport): don't pre-allocate mtu on max_datagram_size (#2086) The `neqo_transport::Connection::max_datagram_size` creates an `Encoder`, writes a packet header and a packet number and determines how many bytes of the mtu are left. https://github.com/mozilla/neqo/blob/28f60bd0ba3209ecba4102eec123859a3a8afd45/neqo-transport/src/connection/mod.rs#L3408-L3427 The `Encoder` only has to hold the packet header and the packet number. Yet it is initialized with `Encoder::with_capacity(mtu)`. https://github.com/mozilla/neqo/blob/28f60bd0ba3209ecba4102eec123859a3a8afd45/neqo-transport/src/connection/mod.rs#L3408 Note that `PacketBuilder::short` and `PacketBuilder::long` read the `Encoder::capacity` through `PacketBuilder::infer_limit`. But `PacketBuilder::infer_limit` falls back to `2048` if the capacity is below `64`, which will be the case when using `Encoder::default()` instead of `Encoder::with_capacity(mtu)`. `2048` should be plenty enough for the packet header and the packet number. https://github.com/mozilla/neqo/blob/28f60bd0ba3209ecba4102eec123859a3a8afd45/neqo-transport/src/packet/mod.rs#L152-L180 https://github.com/mozilla/neqo/blob/28f60bd0ba3209ecba4102eec123859a3a8afd45/neqo-transport/src/packet/mod.rs#L188-L225 https://github.com/mozilla/neqo/blob/28f60bd0ba3209ecba4102eec123859a3a8afd45/neqo-transport/src/packet/mod.rs#L135-L141 This commit prevents the wasted allocation by using `Encoder::default()` instead of `Encoder::with_capacity(mtu)`. Co-authored-by: Lars Eggert --- neqo-transport/src/connection/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index d8d9db2422..3ec406f8da 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -3425,7 +3425,7 @@ impl Connection { }; let path = self.paths.primary().ok_or(Error::NotAvailable)?; let mtu = path.borrow().plpmtu(); - let encoder = Encoder::with_capacity(mtu); + let encoder = Encoder::default(); let (_, mut builder) = Self::build_packet_header( &path.borrow(), From 55e3a9363c28632dfb29ce91c7712cab1f6a58da Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 25 Sep 2024 10:49:17 +0200 Subject: [PATCH 2/2] Extend panic when pn_len is 0 with metadata (#2134) `Connection::add_packet_number -> PacketBuilder::pn -> Encoder::encode_uint` panics when `pn_len` is `0`. These panics are seen in Firefox crash reports. To be able to find the root cause of the panic, add additional metadata. See https://github.com/mozilla/neqo/issues/2132 for details. --- neqo-transport/src/connection/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 3ec406f8da..0ccf854c91 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2018,7 +2018,10 @@ impl Connection { // Count how many bytes in this range are non-zero. let pn_len = mem::size_of::() - usize::try_from(unacked_range.leading_zeros() / 8).unwrap(); - // pn_len can't be zero (unacked_range is > 0) + assert!( + pn_len > 0, + "pn_len can't be zero as unacked_range should be > 0, pn {pn}, largest_acknowledged {largest_acknowledged:?}, tx {tx}" + ); // TODO(mt) also use `4*path CWND/path MTU` to set a minimum length. builder.pn(pn, pn_len); pn