feat: use Cubic CC by default #2295
Conversation
Firefox uses Cubic by default: ``` yaml - name: network.http.http3.cc_algorithm type: RelaxedAtomicUint32 value: 1 mirror: always rust: true ``` https://searchfox.org/mozilla-central/rev/f9517009d8a4946dbdd3acd72a31dc34fca79586/modules/libpref/init/StaticPrefList.yaml This commit updates Neqo to use Cubic instead of New Reno by default.
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 7f8136e. 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
|
|
Cubic does not seem to grow the cwnd as fast as New Reno in congestion avoidance phase. More particularly, while our New Reno implementation increases its cwnd in each iteration by the SMSS, our Cubic implementation only does so in every second iteration. neqo/neqo-transport/src/connection/tests/cc.rs Lines 286 to 287 in 922d266 That is surprising to me. I expected Cubic to grow faster than New Reno in the beginning, given its concave increase early in congestion avoidance. I will give this more thought. |
That seems like a bug: https://datatracker.ietf.org/doc/html/rfc9438#section-1-5
|
|
My assessment above is wrong for the following reason:
Wrong.
Correct. BUT, while New Reno halves its congestion window on a congestion event: neqo/neqo-transport/src/cc/new_reno.rs Line 45 in 9e5a622 Cubic will reduce it by 30% only: neqo/neqo-transport/src/cc/cubic.rs Lines 23 to 25 in 9e5a622 neqo/neqo-transport/src/cc/cubic.rs Lines 206 to 207 in 9e5a622 Thus, within the 5 iterations of the test, Cubic does not grow its congestion window as fast as New Reno, because it starts off with a larger congestion window after the congestion event, i.e. has a head start. |
|
Aside: Is there any point at having |
I assume the split is due to its usage with the neqo/neqo-transport/src/cc/cubic.rs Lines 206 to 207 in 9e5a622 I don't have an opinion on whether it is worth the complexity it introduces. Given our work ahead (#1912) I suggest we keep it as is for now. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2295 +/- ##
==========================================
+ Coverage 95.29% 95.33% +0.03%
==========================================
Files 114 114
Lines 36856 36843 -13
Branches 36856 36843 -13
==========================================
+ Hits 35123 35124 +1
+ Misses 1727 1711 -16
- Partials 6 8 +2 ☔ View full report in Codecov by Sentry. |
| fn cc_cong_avoidance_recovery_period_to_cong_avoidance() { | ||
| let mut client = default_client(); | ||
| let mut server = default_server(); | ||
| fn cc_cong_avoidance_recovery_period_to_cong_avoidance(cc_algorithm: CongestionControlAlgorithm) { |
There was a problem hiding this comment.
This is the only test in cc.rs that specifically depends on the underlying CongestionControlAlgorithm used. The other tests focus on the high level ClassicCongestionControl.
For the above reason, I think it is worth running this unit test with both NewReno and Cubic.
Previously this test would assert on the concrete individual NewReno window increases. That is difficult to do when supporting two CongestionControlAlgorithms.
Instead I suggest simply asserting that after x rounds, the congestion controller reaches the congestion window before the loss event again. Thus it switched from Recovery to CongestionAvoidance.
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
|
Curious what the benchmark results will be. |
Firefox uses Cubic by default:
https://searchfox.org/mozilla-central/rev/f9517009d8a4946dbdd3acd72a31dc34fca79586/modules/libpref/init/StaticPrefList.yaml
This commit updates Neqo to use Cubic instead of New Reno by default.