Store sent packets in a ring buffer instead of a BTreeMap

[ojhermann]

Fixes #2720.

Problem

PacketSpace::sent_packets is inserted on every sent packet and removed on ACK/loss. Because packet numbers are monotonic, a BTreeMap spends most of its cost allocating and freeing a tree node per entry.

Heap-profiling a 200 MiB loopback bench bulk transfer with dhat and deep backtraces (see the confirmation in #2720) attributes 35.8% of all allocations — 24,288 of 67,766 blocks — and ~99% of all BTreeMap-node allocations to this one map, while only ~46 entries are live at peak. It's allocation churn, not residency: CPU spent in the allocator, invisible in RSS, and felt most on edge devices at high packet rates.

Change

Replace the BTreeMap with SentPackets, a sparse ring buffer indexed by packet number - offset (a VecDeque<Option<T>> plus a base offset):

• insert — packet numbers are monotonic, so this is an append: amortized O(1), no per-packet allocation. Skipped packet numbers (PacketNumberFilter) leave vacant slots.
• remove — O(1); vacant slots at the front are reclaimed as the low end is acknowledged, so the buffer tracks the live in-flight window rather than growing without bound.
• get / index / range / values / values_mut / into_values — same surface and semantics as before.

The type keeps a BTreeMap-compatible API, so the call sites are unchanged apart from &pn → pn in two loops. Crucially, range() yields present entries in ascending packet-number order, so the loss-detection loop's gap detection (prev_packet != Some(pn - 1)) and the ACK range walk behave identically — a removed or skipped packet number is absent either way.

Result

Re-profiling the same transfer after the change:

	before	after
sent_packets allocations	24,288 blocks (35.8%)	18 blocks (0.04%)
all BTreeMap-node allocations	24,600	137
total allocations	67,766	40,339 (−40%)

The 18 remaining are the amortized VecDeque growth reallocations (a handful of buffer resizes vs ~24k per-packet node allocs). lost_packets was already negligible (~0.5%) and is left as a BTreeMap.

Testing

• 8 new unit tests on SentPackets: monotonic-with-gaps insert, out-of-order removal, front reclamation, range bounds (including the excluded-start "first entry after x" query used by sent()), drain, mem::take reuse, out-of-range access, and index panic.
• Full quinn-proto suite green (283 tests), including the sim-IO ACK / loss / persistent-congestion tests that depend on iteration order and gap detection.
• cargo fmt and clippy clean.

The dhat harness used for the numbers above is a small feature-gated addition to bench; happy to submit that separately if it'd be useful for future profiling.

Possible follow-ups

Out of scope here, but noting them so they're on record:

• O(1) in-flight lookups. A couple of call sites still scan sent_packets to answer "is anything in flight?" — with the offset-indexed layout these could become O(1) (a live-entry count maintained on insert/remove), removing the last per-poll walks over the window. Happy to do this as a separate PR if wanted.
• Upstream the dhat harness. As above — the feature-gated bench addition could land on its own for future allocation profiling.
  _{This change was written with AI assistance; I've reviewed every line and stand behind it.}

[Ralith]

I have to run before I can give this a thorough review, but I'll note for starters that this is precisely the data structure I had in mind. Thanks!

[ojhermann]

I have to run before I can give this a thorough review, but I'll note for starters that this is precisely the data structure I had in mind. Thanks!

great :) - no rush on my end; hopefully this helps

[djc]

Thanks for working on this?

Have you run the perf tooling to investigate the effect on throughput?

Please avoid posting LLM-generated comments and PR description. See our in-progress CONTRIBUTING with AI policy here:

• #2726

[djc]

• O(1) in-flight lookups. A couple of call sites still scan sent_packets to answer "is anything in flight?" — with the offset-indexed layout these could become O(1) (a live-entry count maintained on insert/remove), removing the last per-poll walks over the window. Happy to do this as a separate PR if wanted.

I would like this in the same PR, as a separate commit.

[ojhermann]

Have you run the perf tooling to investigate the effect on throughput?

Yes, and with positive results.

I ran quinn-perf over loopback, comparing the branch tip against its parent commit. Both binaries built --release from the same toolchain; before/after runs interleaved to cancel drift; 2 GiB transfer, 12 s/run, warmup interval dropped.

Median loopback throughput on an 8-core machine:

• download (server→client): 3.80 → 4.12 Gbps (~+8%); slowest ring-buffer run beat the fastest baseline run
• upload (client→server): 3.81 → 3.96 Gbps (~+5%); noisier than download but consistently favoured the change

In agreement with the profiling: removing per-packet allocator churn, so it shows up as CPU headroom / throughput rather than RSS.

[ojhermann]

@djc I've pushed some changes related to your comments, each as a single commit.

I'm going to run the throughput tests again to confirm the performance gains remain; I'll update after that's done.

[djc]

We'll want all of your intermediate commits to be squashed -- we rebase on merge so we want the PR history to be clean, with review feedback addressed in the originating commit.

[ojhermann]

Re-ran the loopback throughput (same method as before) after the review changes, comparing the branch head against the pre-PR state (BTreeMap):

• download: 3.87 → 4.18 Gbps (~+8%), fully separated
• upload: 4.07 → 4.29 Gbps (~+5%), noisier but mean/median both favour the change

So the refactors and the O(1) has_in_flight haven't regressed throughput; the improvement over the BTreeMap baseline still holds.

[djc]

We'll want all of your intermediate commits to be squashed -- we rebase on merge so we want the PR history to be clean, with review feedback addressed in the originating commit.

(To be clear, still good to keep the O(1) changes in a separate commit.)

[ojhermann]

Two commits:

• the original work
• the O(1) work

Also rebased onto the current upstream base.

[djc]

LGTM, thanks!

[ojhermann]

LGTM, thanks!

cool :)

I've resolved the nit, so once CI is green again, it should be OK for a merge.

[djc]

I've resolved the nit, so once CI is green again, it should be OK for a merge.

Will wait for a review from @Ralith first.

[Ralith]

This is great, thank you!