fix: RTX more data during handshake #2119
Merged
Conversation
This file contains 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
We don't track which packets are coalesced with others, so when we detect a loss in one packet number space, we cannot resend any coalesced packets in other packet number space. This PR tries to approximate this behavior by scheduling un-ACKed Handshake and 0-RTT packets for RTX when packets are lost in the Initial space. Broken out of mozilla#1998
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Failed Interop TestsQUIC Interop Runner, client vs. server 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2119 +/- ##
=======================================
Coverage 95.35% 95.35%
=======================================
Files 112 112
Lines 36316 36320 +4
=======================================
+ Hits 34628 34632 +4
Misses 1688 1688 ☔ View full report in Codecov by Sentry. |
Benchmark resultsPerformance differences relative to b780e53. coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [99.226 ns 99.524 ns 99.834 ns]
change: [-0.6026% +0.3744% +1.6733%] (p = 0.56 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [117.11 ns 117.41 ns 117.75 ns]
change: [-0.5859% -0.0411% +0.4490%] (p = 0.88 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [116.76 ns 117.13 ns 117.60 ns]
change: [-0.6542% +0.0450% +0.7609%] (p = 0.91 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [97.222 ns 102.09 ns 112.94 ns]
change: [-0.5082% +2.4398% +7.7019%] (p = 0.40 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.58 ms 111.72 ms 111.95 ms]
change: [+0.4681% +0.7206% +1.0067%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: No change in performance detected. time: [25.716 ms 26.798 ms 27.882 ms]
change: [-6.6379% -1.6237% +4.1241%] (p = 0.55 > 0.05)
transfer/pacing-true/varying-seeds: No change in performance detected. time: [33.477 ms 35.139 ms 36.798 ms]
change: [-12.809% -6.5722% -0.0778%] (p = 0.05 > 0.05)
transfer/pacing-false/same-seed: 💚 Performance has improved. time: [25.783 ms 26.532 ms 27.297 ms]
change: [-20.290% -17.377% -14.280%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [38.935 ms 40.757 ms 42.613 ms]
change: [-11.239% -4.4041% +3.3950%] (p = 0.24 > 0.05)
1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected. time: [113.56 ms 114.41 ms 115.77 ms]
thrpt: [863.81 MiB/s 874.06 MiB/s 880.60 MiB/s]
change:
time: [-0.3349% +0.5273% +1.9416%] (p = 0.40 > 0.05)
thrpt: [-1.9046% -0.5246% +0.3360%]
1-conn/10_000-parallel-1b-resp (aka. RPS)/client: No change in performance detected. time: [313.18 ms 316.88 ms 320.65 ms]
thrpt: [31.186 Kelem/s 31.557 Kelem/s 31.931 Kelem/s]
change:
time: [-1.5177% +0.1326% +1.7679%] (p = 0.87 > 0.05)
thrpt: [-1.7372% -0.1324% +1.5411%]
1-conn/1-1b-resp (aka. HPS)/client: Change within noise threshold. time: [33.946 ms 34.148 ms 34.369 ms]
thrpt: [29.096 elem/s 29.285 elem/s 29.458 elem/s]
change:
time: [-1.8744% -0.9375% +0.0083%] (p = 0.05 < 0.05)
thrpt: [-0.0083% +0.9464% +1.9102%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
larseggert
changed the title
mxinden
approved these changes
Sep 16, 2024
Well, it would make it possible to RTX the exact information we deem lost, rather than guessing. I don't know if it's worth it. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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.
We don't track which packets are coalesced with others, so when we detect a loss in one packet number space, we cannot resend any coalesced packets in other packet number space. This PR tries to approximate this behavior by scheduling un-ACKed Handshake and 0-RTT packets for RTX when packets are lost in the Initial space.
Broken out of #1998