Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3674 +/- ##
==========================================
+ Coverage 96.84% 96.97% +0.12%
==========================================
Files 118 118
Lines 39989 41921 +1932
Branches 39989 41921 +1932
==========================================
+ Hits 38728 40653 +1925
- Misses 1237 1244 +7
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Merging this PR will degrade performance by 2.66%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode 1000 STREAM frames |
179.6 µs | 226.4 µs | -20.69% |
| ❌ | Simulation | decode 100 STREAM frames |
19.2 µs | 24 µs | -19.94% |
| ❌ | Simulation | decode 10 STREAM frames |
3.3 µs | 3.9 µs | -14.74% |
| ⚡ | WallTime | walltime/pacing-false/same-seed |
2.1 ms | 2 ms | +4.26% |
| ⚡ | Simulation | simulated/pacing-false/varying-seeds |
75.7 ms | 72.7 ms | +4.09% |
| ⚡ | WallTime | walltime/pacing-true/same-seed |
2.1 ms | 2 ms | +3.67% |
| ⚡ | WallTime | walltime/pacing-false/varying-seeds |
2.1 ms | 2 ms | +3.53% |
| ⚡ | WallTime | walltime/pacing-true/varying-seeds |
2.1 ms | 2 ms | +3.47% |
| ⚡ | WallTime | walltime/1000-streams/each-1-bytes |
3.8 ms | 3.7 ms | +3.27% |
| ⚡ | WallTime | walltime/1000-streams/each-1000-bytes |
12.6 ms | 12.3 ms | +2.68% |
| ⚡ | WallTime | walltime/10-streams/each-1048576-bytes |
29.2 ms | 28.5 ms | +2.43% |
| ⚡ | WallTime | neqo-s2n |
50.3 ms | 49.3 ms | +2.13% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mvanhorn:fix/3641-neqo-ack-32-range-limit (402d83f) with main (f819183)
Signed-off-by: Lars Eggert <lars@eggert.org>
Deduplicate the tracker setup, ACK frame write, token range assertion, and frame decode logic repeated across the new ACK-range tests into shared helpers, as requested in review.
|
Thanks for testing it over the emulated path, that's great to hear. Done in 05b05c5: the repeated tracker setup, ACK frame write, token range assertion, and frame decode logic are now shared helpers, which removed about 60 duplicated lines from the new tests. |
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
In looking at this changeset, I don't think that it approaches the problem from the right angle. When things are an uncertain length and we have a cap, we should only need to rewind the encoding by a single item at the point that the frame overruns the limit. The O(n^2) linear search for the right size is not the right way to build this.
|
@martinthomson thanks for looking. One correction and then I think we agree on the direction. The sizing loop is not an O(n^2) search. The real weakness is the one your approach fixes: 16 bytes per range is a worst case, so on typical ranges the frame is packed well under the limit and we send fewer ranges than would fit. The wrinkle with rewinding a single item is that the frame writes the extra-range count as a varint before any of the ranges, and its width depends on the final count. Dropping the last range can shrink that field, so the rewind is not purely local to the tail. It is handleable, either by reserving the varint at its estimated width or by re-encoding the header after the count settles. If you are happy with the latter, I will rework it to encode ranges until the frame overruns the limit, rewind the last one, and fix up the count, which drops the pre-computation entirely and packs tighter than this version. I will also rebase, the branch is behind main. |
db961ec to
0eaaf2c
Compare
Performance profiles for profiler.firefox.comBenchmarks (14)
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The space calculations handle varint boundaries correctly and the targeted tests cover the affected behavior.
Review effort: Balanced
Findings: None
What changed in this PR
Removes the 32-range ACK cap, sizing ACK frames by available packet space while preserving room for another frame.
Changes:
- Raises tracked ACK ranges to 128.
- Accounts for variable-length range-count encoding and packet capacity.
- Adds coverage for large, constrained, and 64+ range ACKs.
| File | Description |
|---|---|
neqo-transport/src/tracking.rs |
Updates ACK range budgeting and adds boundary tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codspeedbot fix this regression |
I looked into this and there is no regression to fix: the reported deltas come from the benchmark CI runner migration, not from this PR. Evidence Your head run (
The base used in the report comes from an older environment. Comparing That also explains the odd shape of the report: a change confined to No code changes made — the branch is untouched at One observation while checking: the ACK path this PR changes is not currently exercised by the benchmark suite. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
|
Thanks @larseggert. CodSpeedBot looked into it and found no regression attributable to this PR: head 0eaaf2c and main at 9f9c801, measured on the same macro runners, give identical numbers (coalesce_acked_from_zero 1000 ranges at 655.386 us on both, peak memory 2,443,591 B on both). A main-vs-main comparison across the runner migration (fb2eb16 to 9f9c801) reproduces the same 10 regressions and 6 improvements with zero source changes in between, so the report's base was just measured in the old environment. The remaining red checks are the Vendor into Gecko jobs, which are currently failing on every branch including the merge-queue run for #4016 on main, so those look unrelated too. One thing CodSpeedBot noticed that is worth recording: the ACK path this PR changes is not exercised by the bench suite. transfer_simulated drops packets through tail-drop queues, so losses arrive as contiguous bursts and never reach the old 32-range cap. If you want CI coverage for the many-small-holes pattern from #3641, that needs a simulated path with sparse random loss. Happy to add one in a follow-up if you think it is worth having. Anything else you want before this goes in? |
|
@mvanhorn I think you wanted to rewrite this to implement @martinthomson's go-back-1-on-overrun design? |
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Yes, thanks @larseggert. Reworked to @martinthomson's design in 332a686. Ranges are encoded directly into the frame one at a time. When the next range would overrun the limit, that single range is rolled back and encoding stops. The range-count varint is patched afterwards, and the check accounts for it growing. The O(n²) size search is gone, and there's a test for an overrun mid-list dropping exactly one range. |
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
See inline. Also, the tests could be more DRY.
Move the in-place varint rewrite into neqo-common as Encoder::rewrite_varint. Reserve the ACK Range Count at the width the pending ranges could need and write the final count into it, non-minimally if needed, so encoded ranges never move. ACKs with fewer than 64 pending ranges keep a 1-byte count. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Thanks, all three are in c367ebc:
The red CI jobs look unrelated: NetBSD's |
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
Thanks for the recent updates. One more round please.
With 64 or more pending ranges the count was reserved at two bytes even when the packet only had room for fewer ranges, and the base-length check then skipped the frame. Pick the ranges that fit first and encode the count at its minimal width. Builder::write_len now uses Encoder::rewrite_varint, and the test helper measures encoder output instead of restating the length formula. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015XShqeJJr3336iNwLibpXz Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Thanks for another round. 5405168 covers the inline points: the count is now sized from the ranges that actually fit, so the 65-pending case writes a one-byte count instead of bailing; write_len calls Encoder::rewrite_varint; acked uses with_capacity; and the length helper measures real encoder output. On MAX_TRACKED_RANGES: an ACK-only packet has room for about 123 single-packet ranges, so at 32 we were dropping gaps we could have reported in a single ACK. 128 covers that. On cost: add() checks the newest range first, so in-order packets return immediately, and the walk only happens on reordering. Happy to go back down to a smaller bound if you'd rather keep that walk shorter. |
| /// | ||
| /// We don't send ranges that have been acknowledged, but they still need | ||
| /// to be tracked so that duplicates can be detected. | ||
| fn write_frame<B: Buffer>( |
There was a problem hiding this comment.
This function is now extremely long and complicated. How can it be factored better?
| } | ||
| }, | ||
| ); | ||
| let max_extra = pending.clone().count(); |
There was a problem hiding this comment.
OK, a lot of feedback here. This counting of the length is the source of a lot of extra complexity.
Second, this reserves just two bytes for other frames, which seems excessive. A better design would be to cap the size to (maybe) 512 bytes. At 512 bytes, you can guarantee that the range count is a single byte, which allows you to use something like Encoder::encode_vvec_with for encoding ranges without a performance hit.
The inner closure passed to that could do the size limiting by looping until it doesn't have space (you could do this loosely by writing until there is less than 496 bytes remaining to avoid having to count sizes on every range, but any value smaller than 512 will preserve the one byte length guarantee). Compute that limit by looking at the minimum of encoder.remaining() (minus any ECN count size) and a smaller value, like 512 (I'd prefer smaller still, but this would be the maximum). To avoid counting the size of the last range, you could assume that that one is 16 bytes long pretty safely.
Thus, the algorithm would look approximately like:
let Some(space) = builder.remaining().checked_sub(fixed_len()) else {
// No space for even the fixed part. Abort.
return;
};
// Note: I would factor the closure out.
// It receives a reference to `self.ranges`, so you'd need a `Self::foo` function.
let mut acked = Vec::new(); // we might want to look at pre-allocation, but not right now
builder.encode_frame(frame_type, |b| {
b.encode_varint(first.largest);
//... other fixed fields
b.encode_varint(first.largest);
b.encode_varint(ack_delay);
// See below for how this works.
let range_count_reservation = b.reserve::<1>();
b.encode_varint(first.len() - 1);
// Note: I'd factor this block out as well.
// As above, the reference to `self.ranges` means a `Self::foo` function.
{
// Require space for a full-sized gap plus range.
// ACK frames are sent first in most cases, so it is unlikely that this will be a problem.
// This is better than counting the size of every range.
let range_space = min(Self::RANGE_FRAME_SPACE, space).saturating_sub(16);
let stop_after = b.len() + range_space;
let mut last = first.smallest;
for range in pending {
if b.len() > stop_after { break; }
b.encode_varint(last - range.largest - 2);
b.encode_varint(range.len() - 1);
last = range.smallest;
acked.push(range.clone());
}
// Writing in at a given offset requires new code in `Encoder`.
b.overwrite(range_count_reservation, [u8::try_from(acked.len())?]);
// This could be made safer by having a "reserve()" function that returns an opaque
// type that holds a reference to the range that was reserved.
// Clues: `struct Reservation<const N: usize>(usize);`
// and `pub fn overwrite<const N: usize>(&mut self, rsv: Reservation<N>, data: [u8; N]) { ... }`.
if let Some((ect0, ect1, ce)) = ecn {
// write those out
}
});| // the extra ranges that fit. Sizing it from every pending range uses two | ||
| // bytes once 64 extras are pending, and that extra byte can push the base | ||
| // frame over the limit when the packet has room for fewer ranges. | ||
| let limit = builder.limit().saturating_sub(Self::ACK_FRAME_RESERVE); |
There was a problem hiding this comment.
This needs to use Builder::remaining(). We send ACK frames before any others in most cases, but that's not a guarantee we want to bake in.
Follow the design from review: write the fixed ACK fields, reserve a one-byte ACK Range Count, then write extra ranges until fewer than 16 bytes of the range budget remain, and overwrite the count at the end. The range budget is the smaller of 512 bytes and Builder::remaining() less the fixed fields and ECN counts, so the count always fits in one byte and no range is measured before it is written. Ranges that do not fit wait for a later ACK. The fixed fields and the range loop are factored into encode_ack_fixed and encode_ack_ranges. neqo-common gains Encoder::reserve and Encoder::overwrite with a Reservation<N> type. USEFUL_ACK_LEN goes back to a one-byte count, as on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFTE4BHbGYzgf8Uhzcppdh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
@martinthomson reworked along the lines you sketched in 402d83f: the fixed fields go through |
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
I like the changes (no surprise). At least one more round. I stopped before looking before I got to tests; plenty of small-scale stuff to that point.
| /// | ||
| /// When `rsv` is not contained in the bytes written to this encoder. | ||
| pub fn overwrite<const N: usize>(&mut self, rsv: Reservation<N>, data: [u8; N]) -> &mut Self { | ||
| assert!(rsv.0 >= self.start, "reservation is outside the encoder"); |
There was a problem hiding this comment.
| assert!(rsv.0 >= self.start, "reservation is outside the encoder"); | |
| debug_assert!(rsv.0 >= self.start, "reservation is outside the encoder"); |
Reservations are produced by this code, so a caller should not be able to produce one that violates these constraints.
| let len = self.len(); | ||
| assert!(end <= len, "reservation extends past encoder"); |
There was a problem hiding this comment.
As above, plus an inlining.
| let len = self.len(); | |
| assert!(end <= len, "reservation extends past encoder"); | |
| debug_assert!(end <= self.len(), "reservation extends past encoder"); |
| enc.overwrite(one, [0x11]); | ||
| enc.overwrite(two, [0x22, 0x33]); |
There was a problem hiding this comment.
| enc.overwrite(one, [0x11]); | |
| enc.overwrite(two, [0x22, 0x33]); | |
| enc.overwrite(two, [0x22, 0x33]); | |
| enc.overwrite(one, [0x11]); |
Just to demonstrate that order doesn't matter.
| stats.largest_acknowledged = first.largest; | ||
| stats.ack += 1; | ||
|
|
||
| let acked = { |
There was a problem hiding this comment.
I'm fairly sure you can drop this let acked = { ... } wrapper.
| let acked = { |
| acked | ||
| }; |
There was a problem hiding this comment.
Continuing the above suggestion...
| acked | |
| }; |
| let mut acked = Vec::new(); | ||
| builder.encode_frame(frame_type, |encoder| { |
There was a problem hiding this comment.
Maybe we can change encode_frame() to return the value of the closure, which would avoid the need for the weird mut variable assignment pattern. (Very optional; I can open an issue to track doing that.)
| let ecn_len = ecn.map_or(0, |(ect0, ect1, ce)| { | ||
| Encoder::varint_len(ect0) + Encoder::varint_len(ect1) + Encoder::varint_len(ce) | ||
| }); |
There was a problem hiding this comment.
I'd inline this variable as well. It's not doing anything on its own.
| let mut acked = Vec::new(); | ||
| builder.encode_frame(frame_type, |encoder| { | ||
| let reservation = Self::encode_ack_fixed(encoder, &first, ack_delay); | ||
| acked = Self::encode_ack_ranges(ranges, encoder, &first, reservation, space); |
There was a problem hiding this comment.
I think you can pass first by value here, which will save cloning it one extra time.
| let Some(first) = self.ranges.iter().find(|range| range.ack_needed()).cloned() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
It would require a change to the function signatures, but I think you can create a filtered iterator for interesting ranges before this, as follows:
| let Some(first) = self.ranges.iter().find(|range| range.ack_needed()).cloned() else { | |
| return; | |
| }; | |
| let mut ack_needed = self.ranges.iter().filter(PacketRange::ack_needed); | |
| let Some(first) = ack_needed.next().map(Clone::clone) else { | |
| return; | |
| }; |
Then you pass the same iterator to encode_ack_ranges.
That avoids iterating over packets that don't need an ACK twice (once here, once when building the packet).
| #[expect( | ||
| clippy::cast_possible_truncation, | ||
| reason = "The fit check guarantees that `value` fits in the selected width." | ||
| )] | ||
| pub fn rewrite_varint<T: Into<u64>>(&mut self, off: usize, len: usize, value: T) -> &mut Self { | ||
| let value = value.into(); | ||
| assert!(matches!(len, 1 | 2 | 4 | 8), "invalid varint length {len}"); | ||
| assert!( | ||
| Encoder::<Vec<u8>>::varint_len(value) <= len, | ||
| "varint value does not fit in {len} bytes" | ||
| ); | ||
| assert!(off + len <= self.len(), "varint extends past encoder"); | ||
| let dest = &mut self.as_mut()[off..off + len]; | ||
| match len { | ||
| 1 => dest[0] = value as u8, | ||
| 2 => dest.copy_from_slice(&(value as u16 | (1 << 14)).to_be_bytes()), | ||
| 4 => dest.copy_from_slice(&(value as u32 | (2 << 30)).to_be_bytes()), | ||
| 8 => dest.copy_from_slice(&(value | (3 << 62)).to_be_bytes()), | ||
| _ => unreachable!("len checked above"), | ||
| } | ||
| self | ||
| } |
There was a problem hiding this comment.
I didn't pick up on this before, but this is an entirely unrelated change. It's not bad, but it is a LOT. I think you should revert this part.
Client/server transfer resultsPerformance differences relative to f819183. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Summary
ACK frames are no longer capped at 32 ranges.
write_frameinneqo-transport/src/tracking.rspreviously truncated toMAX_RANGES = 32unconditionally; it now sizes the range count against the actual space available in the packet, so loss patterns producing many small holes get acknowledged in one frame instead of dropping older ranges, as requested in #3641.Why this matters
larseggert filed #3641 after observing the hard cap: with more than 32 ranges, the peer never learns about older holes and retransmits data the receiver already has. The frame builder already negotiates space byte by byte; the cap was a leftover safety bound. Sizing now accounts for the range-count varint growing to two bytes at 64+ ranges (review round caught that), so
ACK_FRAME_RESERVEstill guarantees room for a following useful frame in tight packets.Testing
Added tracking tests covering: many-range ACK in a roomy packet (all ranges encoded), tight-packet truncation (reserve preserved, frame still parses), and the 64+ range varint boundary.
cargo fmt --check,cargo clippy --all-targets -- -D warnings, and the fullneqo-transportsuite (772 tests) pass locally.AI was used for assistance.
Fixes #3641