-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8371475: HttpClient: Implement CUBIC congestion controller #28195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into |
|
@djelinski This change is no longer ready for integration - check the PR body for details. |
|
@djelinski The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
If jdk.httpclient.quic.congestionController is exposed then we'll need to track this with a CSR. |
vy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/QuicRenoCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicRenoCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java
Outdated
Show resolved
Hide resolved
|
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout quic-cubic
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@djelinski this pull request can not be integrated into git checkout quic-cubic
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
djelinski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AlanBateman @vy for the reviews.
The property was not meant to be exposed; IMO CUBIC is superior to Reno in all aspects that matter. I renamed it to include internal.
We will probably need to revisit the configuration if and when we implement BBR, but we aren't quite there yet.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicRenoCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicRenoCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
|
And another question: after this change - do we still have tests for the Reno congestion controller? Or is everthing using Cubic? |
djelinski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dfuch for the review.
We didn't have any Reno-specific tests. I modified a few of the existing tests to also run with Reno now.
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicBaseCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Outdated
Show resolved
Hide resolved
src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicCubicCongestionController.java
Show resolved
Hide resolved
dfuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes look good to me. Please make sure tests remain stable before integrating.
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.congestionControllertoreno.A new test was added to exercise the new congestion controller. Existing tests continue to pass.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28195/head:pull/28195$ git checkout pull/28195Update a local copy of the PR:
$ git checkout pull/28195$ git pull https://git.openjdk.org/jdk.git pull/28195/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28195View PR using the GUI difftool:
$ git pr show -t 28195Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28195.diff
Using Webrev
Link to Webrev Comment