On Fri, 7 Nov 2025 13:38:23 GMT, Daniel JeliĆski <[email protected]> wrote:
>> CUBIC is a standard TCP congestion control algorithm that uses a cubic
>> function instead of a linear congestion window increase function to improve
>> scalability and stability over fast and long-distance networks. CUBIC has
>> been adopted as the default TCP congestion control algorithm by the Linux,
>> Windows, and Apple stacks.
>
> This PR adds a new congestion controller algorithm. It reuses a large part of
> the QuicRenoCongestionController, which was refactored to two classes -
> QuicBaseCongestionController, containing the shared code, and
> QuicRenoCongestionController, containing only the code that is unique to Reno.
>
> CUBIC is now the default congestion controller. Reno can still be selected by
> setting the system property `jdk.httpclient.quic.congestionController` to
> `reno`.
>
> A new test was added to exercise the new congestion controller. Existing
> tests continue to pass.
Really neat work @djelinski! I liked the surgery you carried out in
`QuicBaseCC`. đŻ
I've dropped some minor remarks. I guess we will need some time to wrap our
mind around the math involved in the PR.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java
line 371:
> 369: private static QuicCongestionController createCongestionController
> 370: (String dbgTag, QuicRttEstimator rttEstimator) {
> 371: String algo =
> System.getProperty("jdk.httpclient.quic.congestionController", "cubic");
Can we keep this internal? That is, can we use
`jdk.internal.httpclient.quic.congestionController` instead?
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
line 41:
> 39: * RFC 9438: CUBIC for Fast and Long-Distance Networks
> 40: */
> 41: public class QuicCubicCongestionController extends
> QuicBaseCongestionController {
I guess this can be `final`.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
line 62:
> 60: public QuicCubicCongestionController(String dbgTag, QuicRttEstimator
> rttEstimator) {
> 61: super(dbgTag, rttEstimator);
> 62: this.rttEstimator = rttEstimator;
Can we make `rttEstimator` a `protected` field of
`QuicBaseCongestionController`?
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
line 112:
> 110: protected boolean congestionAvoidanceAcked(int packetBytes, Deadline
> sentTime) {
> 111: boolean isAppLimited;
> 112: isAppLimited = sentTime.isAfter(lastFullWindow);
Suggestion:
boolean isAppLimited = sentTime.isAfter(lastFullWindow);
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
line 122:
> 120: // target = Wcubic(t)
> 121: // this is less aggressive than RFC 9438, which uses
> target=Wcubic(t+RTT),
> 122: // but seems to work well enough
Shall we also document why we deviate from the RFC?
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicRenoCongestionController.java
line 40:
> 38: * RFC 9002: QUIC Loss Detection and Congestion Control
> 39: */
> 40: class QuicRenoCongestionController extends QuicBaseCongestionController {
I guess thins can be `final` and consequently `protected` modifier can be
removed from the implemented methods.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicRenoCongestionController.java
line 47:
> 45: protected boolean congestionAvoidanceAcked(int packetBytes, Deadline
> sentTime) {
> 46: boolean isAppLimited;
> 47: isAppLimited = congestionWindow > maxBytesInFlight + 2L *
> maxDatagramSize;
Suggestion:
boolean isAppLimited = congestionWindow > maxBytesInFlight + 2L *
maxDatagramSize;
@djelinski, I see you verbatim copied these lines â which is fine, and makes
things easier to review. Nevertheless, I want to double-check: do we need to
guard against any arithmetic overflows here?
test/jdk/java/net/httpclient/quic/CubicTest.java line 42:
> 40: /*
> 41: * @test
> 42: * @run testng/othervm -Djdk.httpclient.HttpClient.log=trace,quic:cc
> CubicTest
Shall we use JUnit instead?
-------------
PR Review: https://git.openjdk.org/jdk/pull/28195#pullrequestreview-3435245896
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504693701
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504645030
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504653829
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504673398
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504672752
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504622773
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504632463
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2504680316
