On Wed, 12 Nov 2025 15:13:18 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. > > 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 24 commits: > > - Update test comments > - Convert CubicTest to JUnit > - Merge declaration and assignment > - More aggressive target growth > - Merge remote-tracking branch 'origin/master' into quic-cubic > - Make classes final > - Rename system property to internal > - Add a system property to select congestion controller > - Implement fast convergence > - Add comments > - ... and 14 more: https://git.openjdk.org/jdk/compare/1f1f7bb4...195b0f89 src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java line 69: > 67: protected long maxBytesInFlight; > 68: protected Deadline congestionRecoveryStartTime; > 69: protected long ssThresh = Long.MAX_VALUE; Can we remove the `protected` keyword on the mutable fields? All the subclasses are in the same package. src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java line 84: > 82: this.timeSource = source; > 83: this.pacer = new QuicPacer(rttEstimator, this); > 84: } Suggestion: protected QuicBaseCongestionController(String dbgTag, QuicRttEstimator rttEstimator) { this(dbgTag, TimeSource.source(), rttEstimator); } // Allows to pass a custom TimeLine when testing QuicBaseCongestionController(String dbgTag, TimeLine source, QuicRttEstimator rttEstimator) { this.dbgTag = dbgTag; this.timeSource = source; this.pacer = new QuicPacer(rttEstimator, this); } src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java line 67: > 65: // for testing > 66: public QuicCubicCongestionController(TimeLine source, > QuicRttEstimator rttEstimator) { > 67: super(source, rttEstimator); Suggestion: super("TEST", source, rttEstimator); src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java line 114: > 112: if (!isAppLimited) { > 113: if (wEstBytes < cwndPriorBytes) { > 114: wEstBytes += Math.max((long) (ALPHA * maxDatagramSize * > packetBytes / congestionWindow), 1); should we assert that congestionWindow is > 2 ? src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java line 124: > 122: // not sure if dblTarget can overflow a long, but 1.5 > congestionWindow can not. > 123: if (dblTargetBytes > 1.5 * congestionWindow) { > 124: targetBytes = (long) (1.5 * congestionWindow); Suggestion: // targetLimit is 1.5 * congestionWindow long targetLimit = congestionWindow + (congestionWindow >> 1) if (dblTargetBytes > targetLimit) { targetBytes = targetLimit; src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java line 129: > 127: } > 128: if (targetBytes > congestionWindow) { > 129: congestionWindow += Math.max((targetBytes - > congestionWindow) * packetBytes / congestionWindow, 1L); Can `(targetBytes - congestionWindow) * packetBytes / congestionWindow` overflow? src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java line 162: > 160: lastFullWindow = congestionRecoveryStartTime; > 161: // ((wmax_segments - cwnd_segments) / C) ^ (1/3) seconds > 162: kNanos = (long)(Math.cbrt((wMaxBytes - congestionWindow) / C / > maxDatagramSize) * 1_000_000_000); should we assert that wMaxBytes >= congestionWindow ? It is not immediately obvious to me that this is guaranteed. In particular: if (congestionWindow < wMaxBytes) { // fast convergence wMaxBytes = (long) ((1 + BETA) * congestionWindow / 2); seems to imply that `wMaxBytes` will be less than congestion window since 1.7 < 2 So we would end up with a negative value for kNanos, which is defined as: > The time period in seconds it takes to increase the congestion window size at > the beginning of the current congestion avoidance stage to Wmax. so presumably negative values for kNanos are not expected? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527297674 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527332413 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527340117 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527400131 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527418638 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527429228 PR Review Comment: https://git.openjdk.org/jdk/pull/28195#discussion_r2527493627
