-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(transport): Use a shorter ECN probe threshold initially #1964
Conversation
This commit adds a test where a client connects to a server over a connection dropping all ECN marked datagrams (ECN blackhole) in both directions, asserting 43 RTT to detect ECN blackhole, disable ECN and eventually establish connection.
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 #1964 +/- ##
=======================================
Coverage 94.96% 94.97%
=======================================
Files 112 112
Lines 36504 36525 +21
=======================================
+ Hits 34667 34689 +22
+ Misses 1837 1836 -1 ☔ View full report in Codecov by Sentry. |
Benchmark resultsPerformance differences relative to b22512c. coalesce_acked_from_zero 1+1 entries: Change within noise threshold.time: [192.25 ns 192.69 ns 193.17 ns] change: [+0.1683% +0.4838% +0.8057%] (p = 0.00 < 0.05) coalesce_acked_from_zero 3+1 entries: No change in performance detected.time: [235.83 ns 236.39 ns 236.98 ns] change: [-0.6289% -0.1046% +0.3952%] (p = 0.70 > 0.05) coalesce_acked_from_zero 10+1 entries: No change in performance detected.time: [236.56 ns 237.15 ns 237.88 ns] change: [-0.9678% -0.3987% +0.1516%] (p = 0.17 > 0.05) coalesce_acked_from_zero 1000+1 entries: No change in performance detected.time: [213.75 ns 213.96 ns 214.18 ns] change: [-1.1868% -0.5490% +0.0930%] (p = 0.10 > 0.05) RxStreamOrderer::inbound_frame(): No change in performance detected.time: [118.61 ms 118.72 ms 118.84 ms] change: [-0.3036% -0.0863% +0.0809%] (p = 0.43 > 0.05) transfer/pacing-false/varying-seeds: No change in performance detected.time: [40.458 ms 42.593 ms 44.736 ms] change: [-6.8707% +0.0713% +7.2035%] (p = 0.99 > 0.05) transfer/pacing-true/varying-seeds: No change in performance detected.time: [52.546 ms 55.536 ms 58.573 ms] change: [-8.1822% -0.8443% +6.8292%] (p = 0.84 > 0.05) transfer/pacing-false/same-seed: No change in performance detected.time: [47.073 ms 48.737 ms 50.340 ms] change: [-3.5406% +1.2385% +6.2402%] (p = 0.61 > 0.05) transfer/pacing-true/same-seed: No change in performance detected.time: [72.021 ms 78.496 ms 84.917 ms] change: [-3.6631% +8.5992% +22.400%] (p = 0.18 > 0.05) 1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected.time: [252.28 ms 262.67 ms 272.67 ms] thrpt: [366.74 MiB/s 380.71 MiB/s 396.39 MiB/s] change: time: [-11.590% -4.0943% +2.7527%] (p = 0.28 > 0.05) thrpt: [-2.6790% +4.2691% +13.109%] 1-conn/10_000-parallel-1b-resp (aka. RPS)/client: No change in performance detected.time: [417.52 ms 420.98 ms 424.42 ms] thrpt: [23.562 Kelem/s 23.754 Kelem/s 23.951 Kelem/s] change: time: [-0.4052% +0.7829% +1.9536%] (p = 0.20 > 0.05) thrpt: [-1.9162% -0.7769% +0.4069%] 1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected.time: [66.929 ms 67.265 ms 67.642 ms] thrpt: [14.784 elem/s 14.867 elem/s 14.941 elem/s] change: time: [-0.5603% +0.1853% +0.9915%] (p = 0.64 > 0.05) thrpt: [-0.9818% -0.1850% +0.5635%] Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
Friendly ping @larseggert. |
Thanks. I'll add a fix so that we stop trying ECN earlier in this case. |
@mxinden WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good to me. Thank you!
Intuitively 6 RTT to detect an ECN black-hole on the client seems fine under the assumption that ECN black-holes are very rare.
I can clean-up the tests later today or tomorrow unless you want to.
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larseggert I cleaned-up the tests. Mind taking another look?
@KershawChang or @martinthomson, could you take a look? This would let us enable ECN in Gecko w/@mxinden's patch. |
Would be nice to get this reviewed and merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
This commit adds a test where a client connects to a server over a connection dropping all ECN marked datagrams (ECN blackhole) in both directions, asserting 43 RTT to detect ECN blackhole, disable ECN and eventually establish connection. Test assumes 20 RTT for client to detect blackhole, 20 RTT for server to detect blackhole and 3 RTT for handshake to be confirmed.
Test for #1925.
How likely are ECN blackholes on today's Internet? Should this block ECN roll-out on Firefox? //CC @larseggert
Bauer et. alia. in "Measuring the State of ECN Readiness in Servers, Clients, and Routers" suggests that ECN blackholes are rare.
Test output