Consider not pre-allocating each UDP datagram in `output_path`

[mxinden]

When generating a new UDP datagram, we currently pre-allocate the maximum datagram size upfront:

neqo/neqo-transport/src/connection/mod.rs

Lines 2449 to 2451 in 5e6e7ba

	// Frames for different epochs must go in different packets, but then these
	// packets can go in a single datagram
	let mut encoder = Encoder::with_capacity(profile.limit());

For small datagrams, this is wasteful.

How wasteful? Not too much according to our requests-per-second (i.e. RPS) benchmark and my local machine.

diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs
index 929982c1..d8fb5403 100644
--- a/neqo-transport/src/connection/mod.rs
+++ b/neqo-transport/src/connection/mod.rs
@@ -2448,7 +2448,7 @@ impl Connection {
 
         // Frames for different epochs must go in different packets, but then these
         // packets can go in a single datagram
-        let mut encoder = Encoder::with_capacity(profile.limit());
+        let mut encoder = Encoder::new();

➜  neqo-bin git:(main) ✗ critcmp main no-pre-alloc
group                                               main                                   no-pre-alloc
-----                                               ----                                   ------------
1-conn/10_000-parallel-1b-resp (aka. RPS)/client    1.01    189.4±4.91ms 51.6 KElem/sec    1.00    188.0±4.12ms 51.9 KElem/sec

That said, this might not represent reality well.

Also note that it is not as easy as simply applying the above patch, given that the Encoder capacity is implicitly used, e.g. in PacketBuilder::infer_capacity:

neqo/neqo-transport/src/packet/mod.rs

Lines 154 to 160 in 5e6e7ba

	fn infer_limit(encoder: &Encoder) -> usize {
	if encoder.capacity() > 64 {
	encoder.capacity()
	} else {
	2048
	}
	}

[martinthomson]

Isn't there a risk that reallocation will cost more than overallocation? Those numbers are very much inconclusive.

[larseggert]

Are we really over-allocating here? Isn't profile.limit() limited by the MTU?

(We do need to eliminate allocations and reallocations, given the flamegraphs, but I'm not sure this one is a main issue...)

[mxinden]

Isn't there a risk that reallocation will cost more than overallocation?

Yes. That said, given Vec's doubling strategy (see e.g. rust-lang/rust#72227), it might not be that expensive.

Those numbers are very much inconclusive.

Agreed.

Are we really over-allocating here? Isn't profile.limit() limited by the MTU?

Yes, profile.limit is limited by the MTU.

Imagine a 1 GB download. The client will mostly send ACKs, i.e. tiny UDP datagrams significantly smaller than the MTU.

[larseggert]

I think I'd prefer using once-allocated "static" buffers here, and changing Encoder to encode into those rather than owning the memory.

[martinthomson]

Reworking Encoder seems like a more productive thing to investigate. Just this week I saw that our code that encodes transport parameters is given a buffer, but it then makes a new one, encodes into that, then copies the result across. All because the Encoder insists on owning the memory it writes into. That might be something that makes sense in some cases, but having a variant (perhaps using traits and type aliases to keep code size and changes contained) that is given a fixed buffer makes sense.

[mxinden]

Thanks for the input.

I think I'd prefer using once-allocated "static" buffers here, and changing Encoder to encode into those rather than owning the memory.

Reworking Encoder seems like a more productive thing to investigate.

Good idea. That would as well simplify #2593. I will prepare a proof-of-concept enabling encoder to write into borrowed memory.