On Fri, 7 Nov 2025 17:24:11 GMT, Volkan Yazici <[email protected]> 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
