Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson and
mxinden
as code owners
September 14, 2026 02:21
This was referenced Sep 14, 2026
Member
Author
This was referenced Sep 14, 2026
Contributor
Merging this PR will degrade performance by 3.1%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | walltime/1000-streams/each-1000-bytes |
11.1 ms | 12.5 ms | -11.2% |
| ❌ | Simulation | simulated/1000-streams/each-1000-bytes |
171.7 ms | 181.1 ms | -5.14% |
| ❌ | WallTime | walltime/10-streams/each-1048576-bytes |
27.6 ms | 28.8 ms | -4.16% |
| ❌ | WallTime | walltime/1-streams/each-4194304-bytes |
10.3 ms | 10.7 ms | -3.25% |
| ❌ | WallTime | 1-conn/1-100mb-req (aka. Upload) |
47.3 ms | 48.9 ms | -3.23% |
| ❌ | WallTime | walltime/1000-streams/each-1-bytes |
3.6 ms | 3.7 ms | -2.77% |
| ❌ | WallTime | 1-conn/1-100mb-resp (aka. Download) |
47.5 ms | 48.7 ms | -2.56% |
| ❌ | WallTime | walltime/1-streams/each-1000-bytes |
127.6 µs | 130.6 µs | -2.28% |
| ❌ | WallTime | walltime/pacing-true/varying-seeds |
2 ms | 2 ms | -2.13% |
| ❌ | Simulation | simulated/pacing-false/varying-seeds |
72.7 ms | 74.3 ms | -2.11% |
| ❌ | Simulation | coalesce_acked_from_zero 1 ranges |
2.6 µs | 2.7 µs | -2.02% |
| ❌ | Simulation | simulated/pacing-false/same-seed |
75.7 ms | 77.2 ms | -2.02% |
| ⚡ | WallTime | 1-conn/10_000-parallel-1b-resp (aka. RPS) |
68.5 ms | 66.4 ms | +3.18% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing users/jesup/hook_up_queue (a372889) with users/jesup/expose_expire_datagrams (0357aa9)1
Footnotes
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Queue backpressure, size validation, outcome reporting, and expiry accounting are currently incorrect or incomplete.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Rewires WebTransport datagrams to the transport connection’s per-session queue.
Changes:
- Adds send-group/order parameters and richer queue outcomes.
- Adds queue lifecycle, expiry, capacity, and statistics plumbing.
- Updates WebTransport tests and connect-udp compatibility handling.
File summaries
| File | Description |
|---|---|
neqo-http3/tests/webtransport.rs |
Updates datagram calls. |
neqo-http3/src/webtransport.rs |
Exposes new queue API and test helpers. |
neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs |
Updates test helper return types. |
neqo-http3/src/features/extended_connect/tests/webtransport/datagrams.rs |
Updates backpressure assertions. |
neqo-http3/src/features/extended_connect/session.rs |
Integrates per-session datagram queues. |
neqo-http3/src/connection.rs |
Adds queue configuration and expiry plumbing. |
neqo-http3/src/connect_udp.rs |
Adapts connect-udp to the new queue. |
Review details
Suppressed comments (3)
neqo-http3/src/features/extended_connect/session.rs:482
- This bypasses the peer's advertised QUIC DATAGRAM size limit. The previous
Connection::send_datagramcall returnedTooMuchDatawhen the fully encoded payload exceededremote_datagram_size;enqueue_datagramperforms no such check, and packet construction only checks available MTU space. Consequently an oversized frame can be transmitted and both public APIs violate their documentedTooMuchDatacontract. Validate the prefixed payload before enqueueing.
let id = match id.into() {
DatagramTracking::None => None,
DatagramTracking::Id(v) => Some(v),
};
let outcome = conn.enqueue_datagram(
neqo-http3/src/features/extended_connect/session.rs:532
Connection::expire_datagramssweeps every per-session queue, not this session's queue. Multiple WebTransport sessions on one connection are supported (and exercised bydatagrams_multiple_session), so the first hash-map entry processed receives the expiry count for all sessions while the others receive zero. The transport expiry API needs to preserve the owning session ID (or offer a scoped sweep) before updating per-session stats.
pub(crate) fn expire_datagrams(
&mut self,
conn: &mut Connection,
now: Instant,
) -> Vec<Option<DatagramId>> {
let expired = conn.expire_datagrams(now);
self.protocol
.record_expired_outgoing_datagrams(u64::try_from(expired.len()).unwrap_or(u64::MAX));
neqo-http3/src/features/extended_connect/session.rs:742
- No
Protocolimplementation overrides this new method, including the WebTransport protocol that ownsSessionStats. Calls torecord_expired_outgoing_datagramstherefore remain a no-op anddatagrams_expired_outgoingcan never increase. Implement the override onwebtransport_session::Sessionand update its counter.
/// Record that `count` outgoing datagrams expired before being sent.
/// A no-op default for protocols that don't track [`SessionStats`].
fn record_expired_outgoing_datagrams(&mut self, _count: u64) {}
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 14, 2026 05:31
b9b2b07 to
b8a0b6f
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 14, 2026 05:31
1f3c5c1 to
40b0996
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## users/jesup/expose_expire_datagrams #3985 +/- ##
=======================================================================
+ Coverage 97.08% 97.10% +0.02%
=======================================================================
Files 114 114
Lines 40949 41061 +112
Branches 40949 41061 +112
=======================================================================
+ Hits 39754 39873 +119
+ Misses 1181 1174 -7
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 14, 2026 17:56
b8a0b6f to
d9b1eba
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 14, 2026 17:56
40b0996 to
95b5b8d
Compare
mxinden
reviewed
Sep 15, 2026
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 16, 2026 03:45
d9b1eba to
a33970b
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 16, 2026 03:45
95b5b8d to
6a29098
Compare
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 16, 2026 17:19
a33970b to
76a8abc
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 16, 2026 17:19
6a29098 to
71651fe
Compare
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 17, 2026 05:01
76a8abc to
af9c4c2
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 17, 2026 05:01
71651fe to
c1ce174
Compare
mxinden
requested changes
Sep 18, 2026
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 20, 2026 19:34
af9c4c2 to
d17f44b
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 20, 2026 19:34
c1ce174 to
69362db
Compare
jesup
added a commit
that referenced
this pull request
Sep 20, 2026
Reword send_datagram's doc: send-group registration is per-session (register_send_group acts on the session, not individual streams), so "registered for this session's streams" overstated what's actually checked. The lazy-registration suggestion on the same PR is left as-is per mxinden's own "not critical" note.
This was referenced Sep 20, 2026
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 23, 2026 06:27
d17f44b to
9d772af
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 23, 2026 06:27
69362db to
65af4bd
Compare
Contributor
Performance profiles for profiler.firefox.comBenchmarks (14)
|
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 23, 2026 23:54
9d772af to
0357aa9
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 23, 2026 23:55
65af4bd to
6c1685e
Compare
mxinden
reviewed
Sep 25, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
Two comments. Otherwise ready to merge.
Send datagrams through the new per-session queue instead of the legacy connection-wide FIFO, for both WebTransport and connect-udp, so send_datagram reports the queue's backpressure outcome and send_group_id/ send_order reach the scheduler. Each process_http3 tick expires every active session's queue, and a locally closed session drops its queue. connect_udp_send_datagram and connect_udp::ServerSession::send_datagram now return DatagramQueueOutcome, like webtransport_send_datagram, instead of collapsing AboveWatermark, Overflowed and Rejected into Ok(false), so a caller can tell a datagram that was queued but should trigger backoff from one that was refused. This changes the connect-udp public signature. connect-udp has no outgoingMaxBufferedDatagrams of its own, so connect-udp sessions, created or accepted, get a fixed high water mark of 10, the depth the legacy queue enforced, and its resume-signal test drives that instead. Tests cover send-order priority and byte-budget eviction through a live WebTransport connection, and that the queue's resume signal still reaches Http3ServerEvent::OutgoingDatagramSpaceAvailable.
jesup
force-pushed
the
users/jesup/expose_expire_datagrams
branch
from
September 25, 2026 15:05
0357aa9 to
7f57f2c
Compare
jesup
force-pushed
the
users/jesup/hook_up_queue
branch
from
September 25, 2026 15:05
6c1685e to
a372889
Compare
Contributor
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
|
Contributor
Client/server transfer resultsPerformance differences relative to 7f57f2c. 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 |
Contributor
Benchmark resultsNo significant performance differences relative to 7f57f2c. All resultstransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1500 time: [49.350 ms 49.460 ms 49.588 ms]
thrpt: [1.9693 GiB/s 1.9745 GiB/s 1.9788 GiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1500 time: [49.609 ms 49.663 ms 49.720 ms]
thrpt: [1.9641 GiB/s 1.9664 GiB/s 1.9685 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severetransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1500 time: [2.9594 ms 2.9608 ms 2.9623 ms]
thrpt: [337.58 B/s 337.74 B/s 337.91 B/s]
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
2 (2.00%) high mildtransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1500 time: [65.170 ms 65.490 ms 65.815 ms]
thrpt: [151.94 Kelem/s 152.69 Kelem/s 153.44 Kelem/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes time: [10.658 ms 10.661 ms 10.664 ms]
thrpt: [375.09 MiB/s 375.20 MiB/s 375.31 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams-flow-controlled/walltime/10-streams/each-1048576-bytes time: [28.563 ms 28.576 ms 28.588 ms]
thrpt: [349.79 MiB/s 349.95 MiB/s 350.10 MiB/s]
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mildstreams/walltime/1-streams/each-1000-bytes time: [129.37 µs 129.93 µs 130.65 µs]
thrpt: [7.2996 MiB/s 7.3398 MiB/s 7.3715 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1000-streams/each-1-bytes time: [3.8853 ms 3.8961 ms 3.9066 ms]
thrpt: [249.98 KiB/s 250.65 KiB/s 251.35 KiB/s]streams/walltime/1000-streams/each-1000-bytes time: [12.335 ms 12.344 ms 12.353 ms]
thrpt: [77.204 MiB/s 77.260 MiB/s 77.315 MiB/s]transfer/walltime/pacing-false/same-seed time: [2.0086 ms 2.0100 ms 2.0117 ms]
thrpt: [1.9418 GiB/s 1.9434 GiB/s 1.9448 GiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/varying-seeds time: [2.0616 ms 2.0627 ms 2.0640 ms]
thrpt: [1.8926 GiB/s 1.8937 GiB/s 1.8948 GiB/s]transfer/walltime/pacing-true/same-seed time: [2.0686 ms 2.0697 ms 2.0709 ms]
thrpt: [1.8862 GiB/s 1.8874 GiB/s 1.8884 GiB/s]transfer/walltime/pacing-true/varying-seeds time: [2.0435 ms 2.0443 ms 2.0451 ms]
thrpt: [1.9100 GiB/s 1.9108 GiB/s 1.9115 GiB/s]
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mildInstructions per cycleCriterion reported no significant timing changes. All benchmarks
Profiles for profiler.firefox.com (62)
Download data for |
mxinden
approved these changes
Sep 28, 2026
Comment on lines
+198
to
+206
| /// A burst exceeding the byte budget, with a mix of send-order priorities, | ||
| /// must evict low-priority datagrams to make room for high-priority ones - | ||
| /// verified through the real `Http3Client` API and a live connection, not | ||
| /// just on a bare `DatagramQueue` in isolation. `DatagramQueueOutcome::Overflowed` | ||
| /// reports only how many were evicted, not which ones, so identity is | ||
| /// checked the same way the receiving peer would: by which content (each | ||
| /// datagram's payload is its own id, as 8 little-endian bytes) actually | ||
| /// arrives - checked against whatever has been delivered so far, since a | ||
| /// full drain of a backlog this size isn't practical in one exchange. |
Member
There was a problem hiding this comment.
Suggested change
| /// A burst exceeding the byte budget, with a mix of send-order priorities, | |
| /// must evict low-priority datagrams to make room for high-priority ones - | |
| /// verified through the real `Http3Client` API and a live connection, not | |
| /// just on a bare `DatagramQueue` in isolation. `DatagramQueueOutcome::Overflowed` | |
| /// reports only how many were evicted, not which ones, so identity is | |
| /// checked the same way the receiving peer would: by which content (each | |
| /// datagram's payload is its own id, as 8 little-endian bytes) actually | |
| /// arrives - checked against whatever has been delivered so far, since a | |
| /// full drain of a backlog this size isn't practical in one exchange. | |
| /// A burst exceeding the byte budget, with a mix of send-order priorities, | |
| /// must evict low-priority datagrams to make room for high-priority ones. |
The why is important, the how is documented through the test itself.
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.