RFR: 8371475: HttpClient: Implement CUBIC congestion controller [v2]

[Daniel Jeliński]

On Fri, 7 Nov 2025 17:24:11 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> Daniel Jeliński has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits:
>> 
>>  - Add a system property to select congestion controller
>>  - Implement fast convergence
>>  - Add comments
>>  - Update test
>>  - Revert: more aggressive window increases
>>  - Test the cubic curve
>>  - Use custom timeline for testing
>>  - Documentation updates
>>  - Cubic tests, more aggressive Reno window increase
>>  - Log K in milliseconds
>>  - ... and 7 more: https://git.openjdk.org/jdk/compare/5191d720...b95950f2
>
> 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?

Yes, that was my intention, I forgot we use the `internal` segment for internal properties. Updated.

> 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`.

Updated.

> 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`?

We could. The Reno congestion controller doesn't use the estimator other than passing it to the pacer, so I decided not to include it in the base class.

> 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);

Updated.

> 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?

We can do better than that and actually implement the equation from the RFC. Updated.

> 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.

Changed the class to final. The protected modifier is still needed on overridden 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?

Suggestion applied.

MaxBytesInFlight is limited by the amount of available memory, and soft-limited to 16M (see `MAX_BYTES_IN_FLIGHT` in base congestion controller). While it can cross the limit briefly, I don't expect it to get anywhere near the arithmetic limits.

> 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?

Updated.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2517986292
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2517986863
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518734798
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518006013
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518442424
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518452038
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518485342
PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2518698935