-
Notifications
You must be signed in to change notification settings - Fork 129
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
CUBIC RFC 9438 #2535
base: main
Are you sure you want to change the base?
CUBIC RFC 9438 #2535
Conversation
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 06e42ce. 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
|
Benchmark resultsPerformance differences relative to 06e42ce. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected.time: [1.5650 s 1.5736 s 1.5822 s] thrpt: [63.202 MiB/s 63.547 MiB/s 63.897 MiB/s] change: time: [-1.1965% -0.4426% +0.3151%] (p = 0.27 > 0.05) thrpt: [-0.3142% +0.4446% +1.2110%] 1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected.time: [361.87 ms 363.24 ms 364.62 ms] thrpt: [27.426 Kelem/s 27.530 Kelem/s 27.634 Kelem/s] change: time: [-0.7730% -0.2405% +0.2773%] (p = 0.37 > 0.05) thrpt: [-0.2766% +0.2411% +0.7790%] 1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: Change within noise threshold.time: [31.621 ms 31.814 ms 32.015 ms] thrpt: [31.236 elem/s 31.432 elem/s 31.625 elem/s] change: time: [+0.1406% +0.9019% +1.7427%] (p = 0.02 < 0.05) thrpt: [-1.7128% -0.8938% -0.1404%] 1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected.time: [2.5040 s 2.5237 s 2.5437 s] thrpt: [39.312 MiB/s 39.624 MiB/s 39.937 MiB/s] change: time: [-1.0334% +0.2145% +1.5098%] (p = 0.74 > 0.05) thrpt: [-1.4873% -0.2140% +1.0442%] decode 4096 bytes, mask ff: Change within noise threshold.time: [11.987 µs 12.016 µs 12.054 µs] change: [-0.9718% -0.5228% -0.1209%] (p = 0.02 < 0.05) decode 1048576 bytes, mask ff: No change in performance detected.time: [3.0568 ms 3.0662 ms 3.0774 ms] change: [-0.4965% -0.0075% +0.4805%] (p = 0.96 > 0.05) decode 4096 bytes, mask 7f: No change in performance detected.time: [20.044 µs 20.106 µs 20.172 µs] change: [-0.1687% +0.1976% +0.5669%] (p = 0.30 > 0.05) decode 1048576 bytes, mask 7f: No change in performance detected.time: [5.4027 ms 5.4156 ms 5.4292 ms] change: [-0.3385% +0.0047% +0.3593%] (p = 0.96 > 0.05) decode 4096 bytes, mask 3f: No change in performance detected.time: [6.3076 µs 6.3276 µs 6.3567 µs] change: [-0.9352% -0.3126% +0.1764%] (p = 0.32 > 0.05) decode 1048576 bytes, mask 3f: No change in performance detected.time: [2.5058 ms 2.5128 ms 2.5212 ms] change: [-0.4599% -0.0650% +0.3832%] (p = 0.74 > 0.05) 1 streams of 1 bytes/multistream: 💚 Performance has improved.time: [71.624 µs 71.861 µs 72.101 µs] change: [-5.7431% -3.8572% -2.4196%] (p = 0.00 < 0.05) 1000 streams of 1 bytes/multistream: Change within noise threshold.time: [25.290 ms 25.324 ms 25.359 ms] change: [-0.0006% +0.2133% +0.4150%] (p = 0.04 < 0.05) 10000 streams of 1 bytes/multistream: Change within noise threshold.time: [1.7052 s 1.7069 s 1.7087 s] change: [+0.2046% +0.3317% +0.4557%] (p = 0.00 < 0.05) 1 streams of 1000 bytes/multistream: Change within noise threshold.time: [73.288 µs 73.935 µs 75.016 µs] change: [-3.7899% -1.9177% -0.0151%] (p = 0.02 < 0.05) 100 streams of 1000 bytes/multistream: No change in performance detected.time: [3.4012 ms 3.4087 ms 3.4166 ms] change: [-0.5796% -0.2818% +0.0148%] (p = 0.07 > 0.05) 1000 streams of 1000 bytes/multistream: Change within noise threshold.time: [145.27 ms 145.35 ms 145.44 ms] change: [+0.0511% +0.1319% +0.2128%] (p = 0.00 < 0.05) coalesce_acked_from_zero 1+1 entries: No change in performance detected.time: [94.501 ns 94.815 ns 95.136 ns] change: [-0.3552% +0.1321% +0.6876%] (p = 0.64 > 0.05) coalesce_acked_from_zero 3+1 entries: No change in performance detected.time: [112.52 ns 112.87 ns 113.27 ns] change: [-0.3478% +0.0601% +0.5107%] (p = 0.80 > 0.05) coalesce_acked_from_zero 10+1 entries: No change in performance detected.time: [112.11 ns 112.54 ns 113.05 ns] change: [-0.5365% -0.0501% +0.3689%] (p = 0.83 > 0.05) coalesce_acked_from_zero 1000+1 entries: No change in performance detected.time: [93.027 ns 93.495 ns 93.987 ns] change: [-0.7569% +0.3867% +1.5575%] (p = 0.52 > 0.05) RxStreamOrderer::inbound_frame(): Change within noise threshold.time: [116.10 ms 116.16 ms 116.22 ms] change: [-0.4232% -0.3596% -0.2891%] (p = 0.00 < 0.05) SentPackets::take_ranges: No change in performance detected.time: [8.3325 µs 8.5428 µs 8.7391 µs] change: [-1.6730% +1.6602% +5.1838%] (p = 0.34 > 0.05) transfer/pacing-false/varying-seeds: Change within noise threshold.time: [35.601 ms 35.679 ms 35.758 ms] change: [-0.6095% -0.3179% -0.0301%] (p = 0.03 < 0.05) transfer/pacing-true/varying-seeds: No change in performance detected.time: [35.765 ms 35.825 ms 35.884 ms] change: [-0.2855% -0.0542% +0.1857%] (p = 0.65 > 0.05) transfer/pacing-false/same-seed: No change in performance detected.time: [35.304 ms 35.355 ms 35.406 ms] change: [-0.3383% -0.1390% +0.0679%] (p = 0.20 > 0.05) transfer/pacing-true/same-seed: Change within noise threshold.time: [36.456 ms 36.569 ms 36.683 ms] change: [+0.6779% +1.0152% +1.3650%] (p = 0.00 < 0.05) Client/server transfer resultsPerformance differences relative to 06e42ce. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
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.
Thanks! Will need more time to work through the comments.
// | ||
// <https://datatracker.ietf.org/doc/html/rfc9002#section-4.7> | ||
// | ||
// QUESTION: Timeout (RTO/PTO) does not apply to us then? |
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.
That is my understanding. See also #2476 where we currently falsely assume a congestion event on PTO.
// UPDATE: Add condition for `self.cc_algorithm == cubic` here to set `ssthresh` and | ||
// `congestion_window` according to RFC 9438. | ||
// | ||
// <https://datatracker.ietf.org/doc/html/rfc9438#figure-5> |
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.
Not sure I am following. cwnd
here is already based on the Cubic reduce_cwnd
function, no?
/// | ||
/// <https://datatracker.ietf.org/doc/html/rfc9438#name-definitions> | ||
/// | ||
/// ACTIONS: Discuss if we want to switch to segments, confirm we are doing all conversions. |
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.
For the protocol, limited previous discussion: #1074 (comment)
This initial draft PR just scopes the update from our implementation to RFC 9438 in code comments, updates RFC references and doesn't change any code yet.
I mostly did this for myself to scope the update and check what changed, but thought I might upload it as a draft already so people can give their input if I might've missed something or need some additional context somewhere.
Notes about what changed are prefixed with the keyword
UPDATE
and open questions that I still need to look into or gather feedback on with the keywordQUESTION
.I'll have to focus on some other things the next weeks, but will chip away on this on the side.
Closes #1912