refactor(transport): move UDP datagram limit calc outside loop#3045
Conversation
There is no need to calculate the UDP datagram limit for each QUIC packet within the same UDP datagram.
There was a problem hiding this comment.
Pull Request Overview
This refactoring moves the UDP datagram size limit calculation outside the packet building loop to avoid redundant calculations. The limit calculation was previously performed for each QUIC packet within the same UDP datagram, but since all packets in a datagram share the same size constraints, this computation only needs to happen once.
- Moved UDP datagram limit calculation outside the packet iteration loop
- Preserved the AEAD expansion subtraction logic but moved it to the packet builder call
- Added clarifying comments about the AEAD expansion handling
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3045 +/- ##
==========================================
- Coverage 93.42% 93.35% -0.07%
==========================================
Files 124 124
Lines 35967 35969 +2
Branches 35967 35969 +2
==========================================
- Hits 33601 33580 -21
- Misses 1522 1546 +24
+ Partials 844 843 -1
|
Client/server transfer resultsPerformance differences relative to 791fd40. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Benchmark resultsPerformance differences relative to 791fd40. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: Change within noise threshold. time: [193.97 ms 194.37 ms 194.86 ms]
thrpt: [513.18 MiB/s 514.48 MiB/s 515.55 MiB/s]
change:
time: [+0.1983% +0.4353% +0.7118%] (p = 0.00 < 0.05)
thrpt: [−0.7067% −0.4334% −0.1979%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: Change within noise threshold. time: [285.68 ms 288.00 ms 290.25 ms]
thrpt: [34.453 Kelem/s 34.723 Kelem/s 35.005 Kelem/s]
change:
time: [+0.3303% +1.3591% +2.5385%] (p = 0.01 < 0.05)
thrpt: [−2.4757% −1.3409% −0.3292%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [28.309 ms 28.414 ms 28.539 ms]
thrpt: [35.040 B/s 35.193 B/s 35.324 B/s]
change:
time: [−0.4254% +0.0181% +0.4910%] (p = 0.94 > 0.05)
thrpt: [−0.4886% −0.0181% +0.4272%]
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: Change within noise threshold. time: [203.24 ms 203.58 ms 204.00 ms]
thrpt: [490.20 MiB/s 491.20 MiB/s 492.03 MiB/s]
change:
time: [+0.0748% +0.3667% +0.6535%] (p = 0.01 < 0.05)
thrpt: [−0.6493% −0.3654% −0.0747%]
decode 4096 bytes, mask ff: No change in performance detected. time: [11.593 µs 11.627 µs 11.668 µs]
change: [−0.8111% −0.0328% +0.7685%] (p = 0.93 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0228 ms 3.0323 ms 3.0436 ms]
change: [−0.2576% +0.1901% +0.6378%] (p = 0.42 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.966 µs 20.022 µs 20.083 µs]
change: [−1.3257% −0.3883% +0.3873%] (p = 0.42 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.0455 ms 5.0589 ms 5.0755 ms]
change: [−0.2574% +0.1043% +0.5058%] (p = 0.60 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [8.2820 µs 8.3201 µs 8.3650 µs]
change: [−0.5700% +0.4105% +1.8430%] (p = 0.58 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.5853 ms 1.5896 ms 1.5952 ms]
change: [−1.8567% −0.7288% +0.1077%] (p = 0.17 > 0.05)
1-streams/each-1000-bytes/wallclock-time: No change in performance detected. time: [583.70 µs 586.73 µs 590.46 µs]
change: [−0.1172% +0.6230% +1.3432%] (p = 0.10 > 0.05)
1000-streams/each-1-bytes/wallclock-time: No change in performance detected. time: [13.664 ms 13.704 ms 13.752 ms]
change: [−0.5619% −0.2425% +0.1292%] (p = 0.20 > 0.05)
1000-streams/each-1000-bytes/wallclock-time: 💚 Performance has improved. time: [48.641 ms 48.833 ms 49.028 ms]
change: [−2.1399% −1.6198% −1.1249%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [87.965 ns 88.360 ns 88.767 ns]
change: [−0.5262% −0.0908% +0.3470%] (p = 0.69 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [106.01 ns 106.35 ns 106.70 ns]
change: [+0.0271% +0.6838% +1.5180%] (p = 0.08 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [105.34 ns 105.72 ns 106.20 ns]
change: [−0.2804% +0.4519% +1.5644%] (p = 0.39 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [88.911 ns 89.056 ns 89.230 ns]
change: [−0.8617% −0.0023% +0.8455%] (p = 1.00 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [108.28 ms 108.34 ms 108.40 ms]
change: [−1.1266% −0.8775% −0.7128%] (p = 0.00 < 0.05)
sent::Packets::take_ranges: No change in performance detected. time: [4.4849 µs 4.5788 µs 4.6662 µs]
change: [−3.2553% +1.1919% +7.1894%] (p = 0.68 > 0.05)
transfer/pacing-false/varying-seeds/wallclock-time/run: Change within noise threshold. time: [25.008 ms 25.052 ms 25.103 ms]
change: [−0.5293% −0.3028% −0.0749%] (p = 0.01 < 0.05)
transfer/pacing-false/varying-seeds/simulated-time/run: No change in performance detected. time: [25.152 s 25.188 s 25.225 s]
thrpt: [162.38 KiB/s 162.61 KiB/s 162.85 KiB/s]
change:
time: [−0.3352% −0.1129% +0.1018%] (p = 0.29 > 0.05)
thrpt: [−0.1017% +0.1130% +0.3363%]
transfer/pacing-true/varying-seeds/wallclock-time/run: Change within noise threshold. time: [25.408 ms 25.469 ms 25.529 ms]
change: [−1.1634% −0.8145% −0.4614%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds/simulated-time/run: No change in performance detected. time: [24.984 s 25.018 s 25.052 s]
thrpt: [163.50 KiB/s 163.72 KiB/s 163.94 KiB/s]
change:
time: [−0.1707% +0.0300% +0.2491%] (p = 0.77 > 0.05)
thrpt: [−0.2485% −0.0300% +0.1710%]
transfer/pacing-false/same-seed/wallclock-time/run: Change within noise threshold. time: [25.335 ms 25.370 ms 25.418 ms]
change: [−1.8344% −1.6088% −1.3807%] (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: Change within noise threshold. time: [26.893 ms 26.921 ms 26.953 ms]
change: [+0.7016% +0.8988% +1.0732%] (p = 0.00 < 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 |
mxinden
requested review from
KershawChang,
larseggert and
martinthomson
as code owners
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 791fd40. 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
|
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
Wonder if we should factor that out into a helper? That function is already very long. (I realize you just move code here, but since we're touching it, why not?)
|
I have a couple of follow-ups. More to come. Preference for separate pull requests. |
There is no need to calculate the UDP datagram limit for each QUIC packet within the same UDP datagram.