-
Notifications
You must be signed in to change notification settings - Fork 126
8369437: [lworld] Split multiple @run statements in compiler tests into separate @test blocks #1681
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
Conversation
…to separate groups
|
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
|
@chhagedorn 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 102 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 |
Webrevs
|
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.
Nice improvements! Looks good to me.
|
Thanks Tobias for your review! |
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.
Looks good, thank you for doing this! I left some nitpicks, as always feel free to disregard.
| */ | ||
|
|
||
| /* | ||
| * @test id=nAVF |
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.
Nitpick/FYI: switching from kebab case to camel case. I don't think this matters at all but wanted to point it out just in case.
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.
Good observation! I used it for "non-atomic" because it still belongs to the "A" and I did not want to confuse it with "N" for Nullable. It's a little unfortunate, though.
| */ | ||
|
|
||
| /* | ||
| * @test id=AII- |
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.
I think trailing - are awkward? Or is there a convention/reason for this?
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.
Good catch! Fixed.
|
Thanks Paul for your review! Pushed a small update. |
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.
LGTM! Thanks again.
|
Thanks for your reviews! /integrate |
|
Going to push as commit c42a95f.
Your commit was automatically rebased without conflicts. |
|
@chhagedorn Pushed as commit c42a95f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch splits jtreg tests with multiple
@runstatements in one@testblock into different blocks. This allows jtreg to do better parallelization.I walked through all
compiler/valhalla/inlinetypestests. Whenever we had 3 or more@runstatements in a single@testblock, I compared the test with execution times on Mach5, mainly tier1. If the test needed more time to execute, I split the block into separate blocks. Otherwise, I left them untouched (we can repeat this process if necessary when we see some more opportunities). Note: I did not change anything else in the tests themselves like loop iterations etc.Changes
@testblocks.othervm/mainintomainwhen there was no flag passed (could be left-over from runs that had-XX:+EnableValhallabefore).@testblocks.Test Execution Time
Since I changed quite a lot of tests and there is no simple way to run tier1-4 only with
compiler/valhalla/inlinetypestests without also running them with flag combos that they normally do not run, I ran it once through these tiers separately. There were no timeouts reported anymore after the timeout factor change back to 4 in Valhalla. Here are the results for the total machine times compared to the last CI runjdk-26-valhalla+1-83(could of course have some variance but the numbers are just an indication):We can see quite some improvements by simply doing that. Note that JDK-8369530 explores improvement opportunities for the actual testing code.
Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1681/head:pull/1681$ git checkout pull/1681Update a local copy of the PR:
$ git checkout pull/1681$ git pull https://git.openjdk.org/valhalla.git pull/1681/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1681View PR using the GUI difftool:
$ git pr show -t 1681Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1681.diff
Using Webrev
Link to Webrev Comment