8294610: java/net/vthread/HttpALot.java is slow on Linux #10504
Conversation
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
@jaikiran This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 30 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
| * @summary Stress test the HTTP protocol handler and HTTP server | ||
| * @requires vm.debug != true | ||
| * @modules jdk.httpserver | ||
| * @library /test/lib | ||
| * @compile --enable-preview -source ${jdk.version} HttpALot.java | ||
| * @comment The test launch command intentionally uses -Dsun.net.httpserver.nodelay=true |
There was a problem hiding this comment.
Maybe replace this "The test runs with -Dsun.net.httpserver.nodelay=true" as the words "launch command intentionally uses" aren't really needed.
There was a problem hiding this comment.
Done. Updated the PR to use the suggested wording.
| * to enable TCP_NODELAY on the sockets "accept()"ed by the HttpServer. This is | ||
| * to avoid (occasional 40 milli seconds) delays in receiving responses from the server. | ||
| * Given the number of requests being fired in this test, the delayed responses can | ||
| * accumulate into longer test run duration |
There was a problem hiding this comment.
It might be clearer to say that "to avoid occasional 40ms delays receiving responses from the server on Linux.". I think you can drop "Given the number ..." sentence.
| * @run main/othervm/timeout=600 | ||
| * --enable-preview | ||
| * -Dsun.net.httpserver.nodelay=true |
There was a problem hiding this comment.
good, I'm happy this issue was found and the HTTP server already has a way to change this option.
| * @summary Stress test the HTTP protocol handler and HTTP server | ||
| * @requires vm.debug != true | ||
| * @modules jdk.httpserver | ||
| * @library /test/lib | ||
| * @compile --enable-preview -source ${jdk.version} HttpALot.java | ||
| * @comment This test runs with -Dsun.net.httpserver.nodelay=true | ||
| * to enable TCP_NODELAY on the sockets "accept()"ed by the HttpServer. This is | ||
| * to avoid occasional 40ms delays receiving responses from the server on Linux. |
There was a problem hiding this comment.
Thanks for the update the bug tag and improving the comment. Just a minor nit is that you might want to re-flow it to avoid 1 short line and two longer lines. Ignore if you want.
There was a problem hiding this comment.
Hello Alan, what you suggest seems fine. So I've now updated the comment accordingly.
| @@ -23,13 +23,18 @@ | |||
|
|
|||
| /** | |||
| * @test | |||
| * @bug 8294610 | |||
There was a problem hiding this comment.
is this tag valid for a test fix -- afaik it is for a "product code fix" i.e. the test is for a fix in the product code identified by the bugid ?
There was a problem hiding this comment.
JEP 425 added a lot of new tests but missed in @bug tag in many cases. So it should have had @bug 8284161.
There was a problem hiding this comment.
Good point. The JBS issue should be marked with noreg-self too.
There was a problem hiding this comment.
Hello Mark, it hasn't always been clear to me when to update the @bug. The contribution guide isn't completely clear either. So I usually update the bug id if the test change is substantial (in the way it affects the test). In this case, I've now taken Alan's suggestion to include the 8284161 (the one which introduced the test for feature testing) along with the current bug id 8294610. I am willing to drop the 8294610 from the @bug if there's some agreement towards it.
Alan has marked the JBS issue as noreg-self, so that's been taken care of.
There was a problem hiding this comment.
I agree with Mark. The @bug tag serves to identify the test that needs to be run for a particular source code change. If the changes are all in the test itself, then there's no need for that information. The JBS issue will have a noreg-self keyword indicating that the test itself needs to be run, and that there's no need for any new non-regression test to test the change.
There was a problem hiding this comment.
Done. Updated the PR to remove 8294610 from the bug id
|
Tests for tier1, tier2, tier3 came back fine. I also verified on linux, windows and macos that the duration of this test is now within 5 to 8 seconds range. |
The test runs in test2. It's good that this issue was tracked down and the execution time is consistent now. |
I realized it too late that this was a test-only change and running the rest of the tiers isn't necessary :) |
|
Thank you everyone for the reviews. |
|
/integrate |
|
Going to push as commit fd59430.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Can I please get a review for this test-only change which proposes to improve the test run duration of
java/net/vthread/HttpALot.javatest, on Linux? This relates to https://bugs.openjdk.org/browse/JDK-8294610Experiments have shown that on Linux, due to delayed TCP ACKs from the client (for some previous sent data/packet), the server sent response waits for the ACKs before sending the response data (in a TCP packet). Each delayed ACK is 40 milli seconds and given the number of requests this test (intentionally) issues, the delay accumulates causing the test to take longer duration.
With the change in this PR, the test completes in around 6 to 8 seconds on a Linux system as compared to 60 odd seconds previously on the same setup. This change hasn't shown any negative impact on macos (which continues to run this test in around 5 to 6 seconds like previously). I will trigger some runs in our CI setup to make sure this works as expected in all other OS too.
As noted in the linked JBS issue, additional experiments to see if anything can be improved in the JDK network layer will be carried out separately, outside the context of this test.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10504/head:pull/10504$ git checkout pull/10504Update a local copy of the PR:
$ git checkout pull/10504$ git pull https://git.openjdk.org/jdk pull/10504/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10504View PR using the GUI difftool:
$ git pr show -t 10504Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10504.diff