refactor(transport): replace magic +1 with named constant#3047
Merged
Conversation
`Connection::max_datagram_size` accounts for the datagram frame type length (i.e. `+ 1`). Instead of an omnious `+ 1`, document it through a named constant.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3047 +/- ##
==========================================
- Coverage 93.42% 93.36% -0.06%
==========================================
Files 124 124
Lines 35967 35977 +10
Branches 35967 35977 +10
==========================================
- Hits 33601 33589 -12
- Misses 1522 1544 +22
Partials 844 844
|
Contributor
|
Bencher Report| Branch | datagram-frame-type |
| Testbed | On-prem |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| google vs. neqo (cubic, paced) | 📈 view plot 🚷 view threshold | 278.67 ms(+0.35%)Baseline: 277.69 ms | 280.32 ms (99.41%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| msquic vs. neqo (cubic, paced) | 📈 view plot 🚷 view threshold | 220.92 ms(+13.67%)Baseline: 194.35 ms | 228.05 ms (96.87%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. google (cubic, paced) | 📈 view plot 🚷 view threshold | 758.49 ms(+0.11%)Baseline: 757.63 ms | 764.86 ms (99.17%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. msquic (cubic, paced) | 📈 view plot 🚷 view threshold | 159.31 ms(+1.30%)Baseline: 157.27 ms | 159.69 ms (99.76%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. neqo (cubic) | 📈 view plot 🚷 view threshold | 91.00 ms(+0.40%)Baseline: 90.64 ms | 94.44 ms (96.36%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. neqo (cubic, paced) | 📈 view plot 🚷 view threshold | 90.30 ms(-1.68%)Baseline: 91.84 ms | 95.55 ms (94.50%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. neqo (reno) | 📈 view plot 🚷 view threshold | 91.17 ms(+0.63%)Baseline: 90.60 ms | 93.95 ms (97.04%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. neqo (reno, paced) | 📈 view plot 🚷 view threshold | 90.52 ms(-1.40%)Baseline: 91.81 ms | 95.41 ms (94.87%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. quiche (cubic, paced) | 📈 view plot 🚷 view threshold | 194.69 ms(+0.47%)Baseline: 193.78 ms | 197.24 ms (98.71%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| neqo vs. s2n (cubic, paced) | 📈 view plot 🚷 view threshold | 223.12 ms(+1.04%)Baseline: 220.83 ms | 223.67 ms (99.75%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| quiche vs. neqo (cubic, paced) | 📈 view plot 🚷 view threshold | 155.71 ms(+2.04%)Baseline: 152.59 ms | 158.24 ms (98.40%) |
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| s2n vs. neqo (cubic, paced) | 📈 view plot 🚷 view threshold | 176.74 ms(+1.61%)Baseline: 173.95 ms | 178.10 ms (99.24%) |
mxinden
requested review from
KershawChang,
larseggert and
martinthomson
as code owners
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the QUIC datagram implementation to replace magic numbers with named constants. It introduces a constant DATAGRAM_FRAME_TYPE_VARINT_LEN to explicitly document the length of datagram frame types in QUIC varint encoding, replacing hardcoded +1 values throughout the codebase.
Key Changes:
- Introduces
DATAGRAM_FRAME_TYPE_VARINT_LENconstant with compile-time assertions - Replaces magic
+1values in datagram size calculations with the named constant - Updates import statements to include the new constant
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| neqo-transport/src/quic_datagrams.rs | Defines the new constant with static assertions and updates datagram frame building logic |
| neqo-transport/src/connection/mod.rs | Imports the new constant and uses it in max datagram size calculation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
larseggert
approved these changes
Oct 14, 2025
Contributor
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to e94d8c6. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Contributor
Client/server transfer resultsPerformance differences relative to e94d8c6. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Contributor
Benchmark resultsPerformance differences relative to e94d8c6. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected. time: [193.93 ms 194.28 ms 194.63 ms]
thrpt: [513.80 MiB/s 514.71 MiB/s 515.65 MiB/s]
change:
time: [−0.3392% +0.0081% +0.3153%] (p = 0.96 > 0.05)
thrpt: [−0.3143% −0.0081% +0.3404%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: Change within noise threshold. time: [287.46 ms 289.12 ms 290.80 ms]
thrpt: [34.388 Kelem/s 34.588 Kelem/s 34.788 Kelem/s]
change:
time: [+0.1301% +0.9370% +1.7736%] (p = 0.02 < 0.05)
thrpt: [−1.7427% −0.9283% −0.1299%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [28.313 ms 28.390 ms 28.479 ms]
thrpt: [35.114 B/s 35.224 B/s 35.319 B/s]
change:
time: [−1.2081% −0.5433% +0.0749%] (p = 0.11 > 0.05)
thrpt: [−0.0749% +0.5463% +1.2229%]
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: 💔 Performance has regressed. time: [202.00 ms 202.32 ms 202.72 ms]
thrpt: [493.28 MiB/s 494.27 MiB/s 495.04 MiB/s]
change:
time: [+1.9426% +2.2155% +2.4790%] (p = 0.00 < 0.05)
thrpt: [−2.4190% −2.1675% −1.9056%]
decode 4096 bytes, mask ff: No change in performance detected. time: [11.586 µs 11.619 µs 11.659 µs]
change: [−0.6872% −0.2662% +0.1161%] (p = 0.21 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0244 ms 3.0357 ms 3.0490 ms]
change: [−0.4047% +0.1069% +0.6422%] (p = 0.71 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.958 µs 20.044 µs 20.159 µs]
change: [−1.1262% −0.4170% +0.1626%] (p = 0.23 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.0484 ms 5.0611 ms 5.0743 ms]
change: [−0.1210% +0.2109% +0.5439%] (p = 0.22 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [8.2561 µs 8.2781 µs 8.3077 µs]
change: [−1.4166% −0.5252% +0.1577%] (p = 0.22 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.5863 ms 1.5919 ms 1.5989 ms]
change: [−0.9800% −0.3536% +0.2609%] (p = 0.30 > 0.05)
1-streams/each-1000-bytes/wallclock-time: No change in performance detected. time: [581.12 µs 582.96 µs 585.13 µs]
change: [−0.9633% −0.4494% +0.0728%] (p = 0.10 > 0.05)
1000-streams/each-1-bytes/wallclock-time: 💔 Performance has regressed. time: [13.655 ms 13.678 ms 13.702 ms]
change: [+1.1727% +1.5065% +1.8241%] (p = 0.00 < 0.05)
1000-streams/each-1000-bytes/wallclock-time: Change within noise threshold. time: [47.980 ms 48.224 ms 48.543 ms]
change: [+0.3244% +0.9790% +1.8227%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [88.095 ns 88.457 ns 88.808 ns]
change: [−0.7143% −0.1254% +0.4424%] (p = 0.68 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [106.03 ns 106.43 ns 106.83 ns]
change: [−4.3619% −1.7345% −0.0597%] (p = 0.13 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [105.20 ns 105.58 ns 106.04 ns]
change: [−0.7985% −0.3268% +0.0758%] (p = 0.15 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [88.713 ns 88.911 ns 89.200 ns]
change: [−0.7862% +0.2642% +1.3808%] (p = 0.63 > 0.05)
RxStreamOrderer::inbound_frame(): No change in performance detected. time: [108.49 ms 108.64 ms 108.85 ms]
change: [−0.1335% +0.0237% +0.2178%] (p = 0.82 > 0.05)
sent::Packets::take_ranges: No change in performance detected. time: [4.4808 µs 4.5520 µs 4.6099 µs]
change: [−4.7997% −1.7619% +1.3399%] (p = 0.26 > 0.05)
transfer/pacing-false/varying-seeds/wallclock-time/run: Change within noise threshold. time: [25.267 ms 25.315 ms 25.371 ms]
change: [−0.9149% −0.6326% −0.3611%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds/simulated-time/run: No change in performance detected. time: [25.152 s 25.187 s 25.223 s]
thrpt: [162.39 KiB/s 162.62 KiB/s 162.85 KiB/s]
change:
time: [−0.0975% +0.1067% +0.3096%] (p = 0.32 > 0.05)
thrpt: [−0.3086% −0.1066% +0.0976%]
transfer/pacing-true/varying-seeds/wallclock-time/run: Change within noise threshold. time: [25.737 ms 25.801 ms 25.866 ms]
change: [−1.1764% −0.8346% −0.5087%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds/simulated-time/run: No change in performance detected. time: [24.993 s 25.035 s 25.078 s]
thrpt: [163.33 KiB/s 163.61 KiB/s 163.89 KiB/s]
change:
time: [−0.1338% +0.1059% +0.3358%] (p = 0.40 > 0.05)
thrpt: [−0.3346% −0.1058% +0.1340%]
transfer/pacing-false/same-seed/wallclock-time/run: Change within noise threshold. time: [25.432 ms 25.464 ms 25.511 ms]
change: [−1.2885% −1.1023% −0.8872%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed/simulated-time/run: No change in performance detected. time: [25.710 s 25.710 s 25.710 s]
thrpt: [159.31 KiB/s 159.31 KiB/s 159.31 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000%] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000%]
transfer/pacing-true/same-seed/wallclock-time/run: No change in performance detected. time: [26.656 ms 26.680 ms 26.711 ms]
change: [−0.1720% −0.0356% +0.1133%] (p = 0.63 > 0.05)
transfer/pacing-true/same-seed/simulated-time/run: No change in performance detected. time: [25.675 s 25.675 s 25.675 s]
thrpt: [159.53 KiB/s 159.53 KiB/s 159.53 KiB/s]
change:
time: [+0.0000% +0.0000% +0.0000%] (p = NaN > 0.05)
thrpt: [+0.0000% +0.0000% +0.0000%]
Download data for |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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.
Connection::max_datagram_sizeaccounts for the datagram frame type length (i.e.+ 1). Instead of an omnious+ 1, document it through a named constant.Previously raised in https://github.com/mozilla/neqo/pull/1201/files#r2414614536.