8260555: Change the default TIMEOUT_FACTOR from 4 to 1 #26749
Conversation
|
👋 Welcome back lkorinth! A progress list of the required criteria for merging this PR into |
|
@lkorinth 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 138 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 |
|
@lkorinth The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
serviceability
Webrevs
|
@lkorinth Can you send me a link to the failure? |
I sent it to you on email. |
I only got the four timeouts described in the description, I got a few other failures as well that was not timeout related. I sent you a link to the test results in an email. |
Would you mind also running tier9 to avoid surprises there. |
…OMP_WINDOW_LENGTH_{1-3}-GC_AFTER_YIELD
|
Added three new |
| @@ -936,7 +936,8 @@ define SetupRunJtregTestBody | |||
| JTREG_ALL_OPTIONS := $$(JTREG_JAVA_OPTIONS) $$(JTREG_VM_OPTIONS) | |||
|
|
|||
| JTREG_AUTO_PROBLEM_LISTS := | |||
| JTREG_AUTO_TIMEOUT_FACTOR := 4 | |||
| # Please reach consensus before changing this. It was not easy changing it to a `1`. | |||
| JTREG_AUTO_TIMEOUT_FACTOR := 1 | |||
There was a problem hiding this comment.
Since the default value of JTREG_AUTO_TIMEOUT_FACTOR set to 1 by default, then the value of JTREG_AUTO_TIMEOUT_FACTOR when run with -Xcomp should be change from 10 to 2.5
There was a problem hiding this comment.
It is unclear to me if the author meant this to be 2.5 more than normal or 10 more than JTREG default, or a multiplier that seems to work. It does not bother me more if it is a 10 then a 2.5, as it needs to have a value that is not the multiplicative identity value. I will not change this, the change I have made is already large enough and I want this to be integrated ASAP.
There was a problem hiding this comment.
It is also something that can be changed later, in a follow up fix.
There was a problem hiding this comment.
Take test test/hotspot/jtreg/compiler/arraycopy/stress/TestStressArrayCopy.java as example.
If there is a bug in jvm with -Xcomp option which will cause this test run time outed. Before this PR, it will take 7200*10 seconds to run this test finish and report time outed failure. But after this PR, it will take 28800*10 seconds to run this test finish and then report timed out failure. I think the 28800*10 senonds is too long and it's unacceptable.
There was a problem hiding this comment.
DELETED - I confused the timeout with the timeout-factor. New comment below.
There was a problem hiding this comment.
I do not think 4x longer timeouts for -Xcomp is unreasonable. I also do not want to make this huge change even bigger. If you would like to change it after the integration I think that would be valuable --- though my guess is that it could be quite a lot of work.
There was a problem hiding this comment.
@sendaoYan this PR changes the default timeoutFactor and so also has to change quite a number of implicit and explicit timeouts. But that doesn't mean that test configs that already set their own timeoutFactor should adjust by the same factor! That just doesn't work for any test with an implicit default timeout.
There was a problem hiding this comment.
Yes, this PR change the default timeoutFactor when the tested JVM options do not contains '-Xcomp', and at the same time also multiplies 4 of the timeout value defined in some tests.
So after this PR, the tests which the timeout value has been multiplied 4 will have more timeout value, when the tested JVM options contains '-Xcomp'.
I do agree this change, what I mean is this change has some side effect.
If you would like to change it after the integration I think that would be valuable --- though my guess is that it could be quite a lot of work.
I think I can try it in a new PR.
There was a problem hiding this comment.
I want to warn you before you put too much energy into it. Changing the -Xcomp timeout factor might have even bigger impact than my change. Also, I have no idea how well that flag is tested in open testing. That is, your change might look good for you --- but might cause havoc for companies doing more extensive testing.
I have still not received green light for integrating my change, because extensive testing is still being run (and other teams are evaluating). I advise against changing the flag. When I evaluate the benefit for the default timeout, it was mainly not the timeout in itself that was the problem, but the fact that most people have no idea that the timeout factor is applied and thus can not create or debug tests in a good way. I hope this helps you, and does not come out as too negative. I just feel that I have put too much energy into this, and I do not hope that struggle for you.
There was a problem hiding this comment.
@lkorinth Thanks for your advice sincerely. I think you are right, we need more evaluate cautiously before start to change the timeoutFactor for -Xcomp.
I had problems doing this, and I just want to say that I have not run tier9 (I have talked to Douglas off-list). |
|
If there are no mayor objections, I will update the copyrights before I leave work today. |
|
Hi Leo, I played a bit with your changes and I observed intermittent timeout failures for the following tests: The first test failing more frequently. Could you please add /timeout=480 to these two tests as well? |
|
Generally, changes with this many changed files are broken down into smaller PRs to make the review easier and more conclusive. In this case, hotspot, jdk, and langtools might have been good groupings. The reviews could be done in parallel and committed with the final change to jtreg when they are all approved. |
Noted. I did it so reviewers could se the change "as a whole". Feel free to review a part of the change! |
Fixed! Thanks for helping! |
|
Thanks! Changes to JNDI / net / httpclient LGTM. |
|
By rough count there are 1300 tests that have an explicit timeout set. This change would reduce the actual applied timeout to a quarter of what was previously used, yet you have only had to bump the timeout value for a fraction of the tests - which I find somewhat (pleasantly) surprising. It may be that many of these timeouts stem from a time when we had much much slower machines, so a refresh might not be a bad thing. It will take some time to see the full effects of this change though. |
Thanks David! And as you said, a few more adjustments will probably be needed when we have run this for a while. |
|
It seems to me like there is a need to automatically collect normal test run times automatically, so we can match the timeout given to any individual test with the normal execution time. After all, the purpose of any timeout on tests is to allow the test to execute normally, but not wait too long in case of a problem that causes the test to take too long (or forever) to run. I realize that this is highly hardware dependent, but test times tend to be Pareto distributed, so a very quick test maybe takes 1 second on fast machines and 10 on slow, and very slow tests maybe take 15 minutes on fast machines and 40 minutes on slow. In the first case, anything above 15 seconds is probably sus, and in the second case, anything above 60 is probably not good either (I'm just adding 50% to the max time). Right now it seems engineers is spending their valuable time giving guesstimates for each individual test. That seems like poorly used time and resources. |
@magicus unfortunately that is often not the case in practice. We can see many tests that normally run very quickly but occasionally run very slow - minutes versus seconds.
For this exercise existing explicit timeout values need to be multiplied by 4 to keep the same absolute timeout value. In addition a number of tests that use the implicit default timeout (120*4) now need an explicit timeout (480 generally). |
That is surprising and a bit disappointing. I guess it is not worth the effort to try and figure out why this is the case; it could probably vary from test to test and be difficult to track for little gain. Still, it makes you wonder. |
| @@ -46,7 +46,7 @@ | |||
| * @test | |||
| * @bug 8204454 8359053 | |||
| * @summary URL.getContent() should return SoundClip for supported formats | |||
| * @run main/othervm -Xmx128m AudioContentHandlers | |||
| * @run main/othervm/timeout=480 -Xmx128m AudioContentHandlers | |||
| */ | |||
There was a problem hiding this comment.
I've looked at our CI and this test has run 80,000 times and only 10 of those have gone > 120 seconds (and only 2 > 145 seconds)
Perhaps I'd see similar for other tests. But I need to hear test-specific reasons for the test-specific boost of 4x from what I think (120) is the default to 480.
Otherwise I'd prefer no change, or a small change, by maybe 1.5x not 4x, and we'll adjust the test when we see evidence that it is not enough.
There was a problem hiding this comment.
@prrace the change maintains the same absolute timeout value for those tests. Before the default of 120 was multiplied by the timeoutFactor of 4 to given 480. Now the value 480 is multiplied by the timeoutFactor of 1 to give 480. And IIRC Leo only did that for tests that demonstrated a timeout with the new default settings (120*1). It is not practical for Leo to investigate every changed test to see if it could get away with a value between 120 and 480. The change just maintains the status quo. Test owners are free to investigate further if they think it worth fine tuning these values.
This changes the timeout factor from 4 to 1. Most of the changes add timeouts to individual test cases so that I am able to run them with a timeout factor of 0.7 (some margin to the checked in factor of one)
In addition to changing the timeout factor, I am also using a library call to parse the timeout factor from the Java properties (I cannot use the library function everywhere as jtreg does not allow me to add @library notations to non test case files).
My approach has been to run all tests, and afterwards updating those that fail due to the timeout factor. The amount of updated test cases is huge, and my strategy has been to quadruple the timeout if I could not directly see that less was needed. In a few places, I have added a bit more timeout so that it will work with the 0.7 timeout factor.
These fixes have been created when I have ploughed through test cases:
JDK-8352719: Add an equals sign to the modules statement
JDK-8352709: Remove bad timing annotations from WhileOpTest.java
JDK-8352074: Test MemoryLeak.java seems not to test what it is supposed to test
CODETOOLS-7903937: JTREG uses timeout factor on socket timeout but not on KEEPALIVE
CODETOOLS-7903961: Make default timeout configurable
After the review, I will update the copyrights.
I have run testing tier1-8. The last time with a timeout factor of 1 instead of 0.7.
I got 4 timing related faults:
This is probably: https://bugs.openjdk.org/browse/JDK-8361370
I am unsure about this one, it has not failed on my runs before, even with a timeout factor of 0.7, maybe I was unlucky.
I updated this to 480 seconds, I finish this fairly fast (~14s) on my not very fast computer, but the Macs that fail are old x86-based ones.
This is a new test that I got on last rebase. I have added a timeout of 480 to it.
In addition to these four tests, I have another one "java/lang/ThreadLocal/MemoryLeak.java" that earlier failed with a timeout factor of 0.7 but did not fail in the last run. I will not update that test case, because the extra time spent is strange and should be looked at. I have created JDK-8352074 on that test case, and I will probably create another bug describing the timeout I got.
From the review of the cancelled "8356171: Increase timeout for test cases as preparation for change of default timeout factor", I have taken a few actions:
I am now re-running tier 1-8.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26749/head:pull/26749$ git checkout pull/26749Update a local copy of the PR:
$ git checkout pull/26749$ git pull https://git.openjdk.org/jdk.git pull/26749/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26749View PR using the GUI difftool:
$ git pr show -t 26749Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26749.diff
Using Webrev
Link to Webrev Comment