Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson,
mxinden and
omansfeld
as code owners
September 14, 2026 02:20
Member
Author
|
This PR is part of a stack of 15 bookmarks:
Created with jj-stack |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Negotiated-size validation and expiry handling currently risk protocol violations, stalled producers, and lost outcomes.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Integrates per-session outgoing QUIC datagram queues with connection packet scheduling and timers.
Changes:
- Adds round-robin session queue scheduling and queue-management APIs.
- Integrates max-age expiry with connection timers.
- Adds tests for scheduling, teardown, and expiry.
File summaries
| File | Description |
|---|---|
neqo-transport/src/quic_datagrams.rs |
Implements per-session queue handling and scheduling. |
neqo-transport/src/connection/mod.rs |
Exposes queue APIs and integrates expiry timers. |
neqo-transport/src/connection/tests/datagram.rs |
Tests queue fairness, teardown, and expiry. |
Review details
Suppressed comments (1)
neqo-transport/src/connection/mod.rs:1233
- This schedules the callback at
timestamp + max_age, butDatagramQueue::expire_oldremoves an item only whenage > max_age(datagram_queue.rs:250). At an exactly-on-time callback the item remains; if congestion prevents sending it,next_delayselects the same instant again, hits theearliest > nowdebug assertion below, and otherwise creates a zero-delay callback loop. Make the expiry predicate and scheduled deadline agree (for example, expire at>= max_age).
if let Some(dgram_time) = self
.quic_datagrams
.next_datagram_expiry(self.datagram_default_max_age())
{
qtrace!("[{self}] Datagram expiry timer {dgram_time:?}");
delays.push(dgram_time);
- Files reviewed: 3/3 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.
This was referenced Sep 14, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## users/jesup/datagram_engine #3983 +/- ##
===============================================================
+ Coverage 97.07% 97.10% +0.03%
===============================================================
Files 114 114
Lines 40743 40916 +173
Branches 40743 40916 +173
===============================================================
+ Hits 39550 39731 +181
+ Misses 1179 1171 -8
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Merging this PR will degrade performance by 2.4%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | 1-conn/1-100mb-resp (aka. Download) |
46.9 ms | 49.3 ms | -4.95% |
| ❌ | Simulation | simulated/pacing-false/same-seed |
72.7 ms | 75.9 ms | -4.21% |
| ❌ | WallTime | 1-conn/1-100mb-req (aka. Upload) |
47.2 ms | 48.8 ms | -3.33% |
| ❌ | Simulation | simulated/pacing-true/same-seed |
72.8 ms | 75.1 ms | -3.03% |
| ❌ | WallTime | neqo-neqo-cubic |
20.2 ms | 20.8 ms | -2.82% |
| ❌ | WallTime | 1-conn/10_000-parallel-1b-resp (aka. RPS) |
66.8 ms | 68.7 ms | -2.76% |
| ❌ | WallTime | walltime/1000-streams/each-1-bytes |
3.6 ms | 3.7 ms | -2.32% |
| ❌ | WallTime | walltime/pacing-false/varying-seeds |
2 ms | 2 ms | -2.15% |
| ❌ | WallTime | walltime/pacing-false/same-seed |
1.9 ms | 2 ms | -2.1% |
| ❌ | WallTime | walltime/pacing-true/varying-seeds |
2 ms | 2 ms | -2.07% |
| ⚡ | WallTime | neqo-s2n |
50.5 ms | 48.8 ms | +3.59% |
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/add_queue_to_connection (2260742) with users/jesup/datagram_engine (1a3c09a)
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 14, 2026 05:31
5cf2dbe to
662dce4
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 14, 2026 05:31
b0a92f6 to
3aed8a2
Compare
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 14, 2026 17:56
662dce4 to
1c52106
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 14, 2026 17:56
3aed8a2 to
01af9c9
Compare
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 14, 2026 19:42
1c52106 to
f4f263a
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 14, 2026 19:42
01af9c9 to
b01f019
Compare
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 14, 2026 23:44
f4f263a to
c8bef30
Compare
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 17, 2026 15:03
af19e15 to
b31aa87
Compare
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
mxinden
reviewed
Sep 18, 2026
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 20, 2026 19:34
b31aa87 to
a547999
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 20, 2026 19:34
e32b60b to
09ee1b3
Compare
This was referenced Sep 20, 2026
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 23, 2026 06:27
a547999 to
7feab9b
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 23, 2026 06:27
09ee1b3 to
1b0fb26
Compare
Contributor
Performance profiles for profiler.firefox.comBenchmarks (14)
|
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 23, 2026 23:54
7feab9b to
6b42740
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 23, 2026 23:54
1b0fb26 to
b18e376
Compare
mxinden
reviewed
Sep 25, 2026
mxinden
left a comment
mxinden
left a comment
Member
There was a problem hiding this comment.
Two blockers. Otherwise this is ready to merge from my end.
New Connection methods: enqueue_datagram, set_datagram_high_water_mark, set_datagram_max_age, datagram_queue_capacity, drop_session_datagrams. QuicDatagrams gains a BTreeMap<StreamId, DatagramQueue> and round-robins across sessions in write_frames one datagram at a time, so a datagram that doesn't fit is left queued (with its original timestamp) rather than round-tripped back through enqueue. Hooks the datagram-expiry deadline into next_delay and expiry itself into process_timer, the same way every other subsystem's timer works. Expiry - on the timer or from a shrunken max age - also fires OutgoingDatagramSpaceAvailable for any queue it unblocks: shedding by age is how one of these queues is expected to drain, and nothing else would ever revisit a queue that emptied without sending. The caller, not this layer, has to keep what it enqueues within the peer's max_datagram_frame_size: only the path MTU is applied at packet-build time. send_datagram is the checked path. QuicDatagrams also keeps a simpler count-bounded queue (send_datagram/remaining_datagram_queue_capacity) for callers that don't need per-session tracking, priority, or age.
jesup
force-pushed
the
users/jesup/datagram_engine
branch
from
September 25, 2026 15:05
6b42740 to
1a3c09a
Compare
jesup
force-pushed
the
users/jesup/add_queue_to_connection
branch
from
September 25, 2026 15:05
b18e376 to
2260742
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 1a3c09a. 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 1a3c09a. All resultstransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1500 time: [48.608 ms 48.650 ms 48.702 ms]
thrpt: [2.0052 GiB/s 2.0073 GiB/s 2.0091 GiB/s]
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severetransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1500 time: [49.947 ms 49.993 ms 50.040 ms]
thrpt: [1.9516 GiB/s 1.9534 GiB/s 1.9552 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1500 time: [3.0032 ms 3.0039 ms 3.0047 ms]
thrpt: [332.81 B/s 332.90 B/s 332.98 B/s]
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1500 time: [66.316 ms 66.576 ms 66.843 ms]
thrpt: [149.61 Kelem/s 150.20 Kelem/s 150.79 Kelem/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes time: [10.191 ms 10.193 ms 10.196 ms]
thrpt: [392.33 MiB/s 392.42 MiB/s 392.52 MiB/s]streams-flow-controlled/walltime/10-streams/each-1048576-bytes time: [27.698 ms 27.709 ms 27.720 ms]
thrpt: [360.75 MiB/s 360.89 MiB/s 361.03 MiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams/walltime/1-streams/each-1000-bytes time: [132.15 µs 132.69 µs 133.38 µs]
thrpt: [7.1499 MiB/s 7.1873 MiB/s 7.2167 MiB/s]
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildstreams/walltime/1000-streams/each-1-bytes time: [3.8164 ms 3.8267 ms 3.8365 ms]
thrpt: [254.55 KiB/s 255.20 KiB/s 255.88 KiB/s]streams/walltime/1000-streams/each-1000-bytes time: [10.940 ms 10.947 ms 10.954 ms]
thrpt: [87.064 MiB/s 87.119 MiB/s 87.173 MiB/s]
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/same-seed time: [1.9871 ms 1.9881 ms 1.9893 ms]
thrpt: [1.9636 GiB/s 1.9648 GiB/s 1.9658 GiB/s]transfer/walltime/pacing-false/varying-seeds time: [2.0309 ms 2.0319 ms 2.0330 ms]
thrpt: [1.9214 GiB/s 1.9225 GiB/s 1.9234 GiB/s]transfer/walltime/pacing-true/same-seed time: [2.1155 ms 2.1172 ms 2.1192 ms]
thrpt: [1.8432 GiB/s 1.8450 GiB/s 1.8465 GiB/s]transfer/walltime/pacing-true/varying-seeds time: [2.0329 ms 2.0341 ms 2.0354 ms]
thrpt: [1.9191 GiB/s 1.9204 GiB/s 1.9215 GiB/s]Instructions per cycleCriterion reported no significant timing changes. All benchmarks
Profiles for profiler.firefox.com (62)
Download data for |
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.