8370315: [IR-Framework] Allow scenarios to be run in parallel#28065
8370315: [IR-Framework] Allow scenarios to be run in parallel#28065dafedafe wants to merge 23 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back dfenacci! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
The full list of tests using IR-Framework scenarios: Maybe we could run some of them concurrently (e.g. the ones that show more than a certain speedup to avoid parallelizing short tests that might waste most of the time spawning threads). |
Webrevs
|
chhagedorn
left a comment
There was a problem hiding this comment.
Overall looks good, thanks for improving this! I left a few suggestions.
Now the only question remaining is which tests would already benefit from using the parallel version. I guess we can investigate that separately.
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
| synchronized (printLock) { | ||
| String output = baos.toString(); | ||
| if (!output.isEmpty()) { | ||
| System.out.println(output); | ||
| } | ||
| } |
There was a problem hiding this comment.
This will probably also not be sorted by scenario index? Could we also just gather it here and then dump it after the stream? Maybe we can put output into Outcome as well as the exceptions by using a ConcurrentSkipListMap<Scenario, Outcome> map in the parallel case or a normal TreeMap in the non-parallel case.
There was a problem hiding this comment.
The idea was to print the output as soon as one process finishes, so that we can follow the progress a bit better (and if there is an TestFormatException we have it printed up to the exception, although this could be done later as well). Of course then it is not sorted...
That said, the output would be cleaner and more readable if we printed it in order as you suggest. Maybe we could even interleave output and exceptions for the same scenario. What do you think? (though I just noticed we throw another TestRunException after printing the exceptions)
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenarios.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Let me file an RFE for that. |
|
@dafedafe This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
chhagedorn
left a comment
There was a problem hiding this comment.
Sorry, I dropped the ball on this - thanks for the updates! Some more comments but then I think it looks good!
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/lib/ir_framework/TestFramework.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenarios.java
Outdated
Show resolved
Hide resolved
| String output = baos.toString(); | ||
| if (!output.isEmpty()) { | ||
| System.out.println(output); | ||
| } |
There was a problem hiding this comment.
We probably also need to do a similar trick as for the exceptions in order to have ordered stdouts for the scenarios?
There was a problem hiding this comment.
That would be a good idea. Adding it...
There was a problem hiding this comment.
@chhagedorn I guess we can drop the printing of exception at the end then. Is that part of what you were suggesting?
There was a problem hiding this comment.
I might have spoken too soon: JTReg seems to collect stdout and stderr and print them out at once at the end of each (JTReg) test. In this case it doesn't make much sense to print out the output of each test as soon as it finishes (it would be better to collect them and print them in order at the end). @chhagedorn, is there possibly a way to make JTReg print the output "on-the-fly" that you are aware of?
There was a problem hiding this comment.
Yes, I agree. Let's just collect everything, stdout and exceptions, and then print them in scenario index order at the end. That probably also simplifies the logic.
is there possibly a way to make JTReg print the output "on-the-fly" that you are aware of?
I'm not aware of such an option but thought it would be useful in the past when having a long running test and I'm actually only interested in some printed messages at the very start.
test/hotspot/jtreg/compiler/lib/ir_framework/shared/TestFormat.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestBadFormat.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
…enarios.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
….java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
|
@dafedafe This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Issue
Today, the only practical ways to run IR Framework scenarios in parallel seems to be:
This makes it a bit cumbersome to use host CPU cores efficiently when running multiple scenarios within the same test.
Change
This change introduces a method
TestFramework::startParallelto execute multiple scenarios concurrently. The implementation:System.outinto a dedicated buffer and flushes it when the task completes to avoid interleaved output between scenarios (Note: only call paths within thecompile.lib.ir_frameworkpackage are modified to per-task output streams.ProcessToolsmethods still write directly tostdout, so their output may interleave).-DForceSequentialScenarios=trueto force all scenarios to be run sequentially.Testing
ir_framework.testsrunsTestDForceSequentialScenarios.javato test forcing sequential testing (checkin the output order) and added a parallel run toTestScenatios.java(as well as addingForceSequentialScenariosflag toTestDFlags.java)As reference: a comparison of the execution time between sequential and parallel of all IR-Framework tests using scenarios on our machines (linux x64/aarch64, macosx x64/aarch64, windows x64 with different number of cores, so the results for a single test might not be relevant) gave me an average speedup of 1.9.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28065/head:pull/28065$ git checkout pull/28065Update a local copy of the PR:
$ git checkout pull/28065$ git pull https://git.openjdk.org/jdk.git pull/28065/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28065View PR using the GUI difftool:
$ git pr show -t 28065Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28065.diff
Using Webrev
Link to Webrev Comment