8313713: Allow CompileCommand flag to specify compilation level#30352
8313713: Allow CompileCommand flag to specify compilation level#30352kirill-shirokov wants to merge 29 commits intoopenjdk:masterfrom
Conversation
…ial draft implementation
…uld_not_inline(). Replace compiler arument with comp_level in DirectivesStack::getMatchingDirective()
…. Fix level checking in WB_IsMethodCompilable()
…n ClearDirectivesFileStackTest. Sort includes. Update copyright year.
…evel is excluded for the method. Fix WB_IsMethodCompilable for levels 1-3
…mpileonly and exclude CompileCommands with a level argument
…tput without WhiteBox
…o communicate with testee via stdin
…se added to CompileLevelParseTest.java. CompileTask::release_direcive() added for ownership clarity. Null pointer crash fixed in CompilationPolicy::select_task()
|
👋 Welcome back kshiroko! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@kirill-shirokov 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
|
vnkozlov
left a comment
There was a problem hiding this comment.
Nice work, thank you. I have few comments.
test/hotspot/jtreg/compiler/compilercontrol/jcmd/ClearDirectivesFileStackTest.java
Show resolved
Hide resolved
vnkozlov
left a comment
There was a problem hiding this comment.
This looks good. I will submit our testing.
test/hotspot/jtreg/compiler/compilercontrol/jcmd/ClearDirectivesFileStackTest.java
Show resolved
Hide resolved
Co-authored-by: Igor Veresov <igor.veresov@oracle.com>
|
@kirill-shirokov my testing shows the new tests you added are failing on all platforms when other flags passed to them: -Xcomp, -XX:-TieredCompilation, -XX:+Use*GC etc. |
|
Looks reasonable. Have you considered to extend method patterns instead to allow compilation level information to be uniformly applied to any command? |
|
compiler/compilercontrol/commands/CompileLevelPrintTest.java failed in all modes (including default without any flags: |
|
compiler/compilercontrol/commands/CompileLevelWBTest.java all subtests failed with tiered off (-XX:-TieredCompilation): |
|
CompileLevelWBTest.java also failed with -Xcomp: |
Yes. Do we have limit check for such values? Similar to |
dafedafe
left a comment
There was a problem hiding this comment.
Thanks for working on this enhancement @kirill-shirokov!
test/hotspot/jtreg/compiler/compilercontrol/commands/CompileLevelParseTest.java
Outdated
Show resolved
Hide resolved
| * & (vm.opt.CompilationMode == "normal" | vm.opt.CompilationMode == null) | ||
| * @library /test/lib | ||
| * @build jdk.test.whitebox.WhiteBox | ||
| * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox |
There was a problem hiding this comment.
Do we need the @run driver for each @test?
There was a problem hiding this comment.
Not sure. ClassFileInstaller is definitely needed (otherwise the test fails to registerNatives() for WB). The order of execution is determined by jtreg (alphabetic?) and @requires criteria. Do you think it is possible to specify some common section in jtreg which is executed before tests in the same file?
There was a problem hiding this comment.
Sorry @kirill-shirokov I think I misread the @run driver line: It is definitely needed if you use the whitebox.
…jtreg/compiler/compilercontrol/commands/CompileLevelWBTest.java
@vnkozlov I think I've fixed the tests. I ran the fixed ones locally on MacOS/Linux/Windows with all combinations of Maybe there are some exotic compiler flags that I didn't cover with my testing - I will be glad to fix tests if any failures discovered. |
|
Okay, I will submit new testing. |
I haven't seen any checking for numeric values in the code. And this is an initial implementation of int and uint OptionType in CompileCommand: |
|
Also, please let me know if the new format needs to be documented (e.g. in |
Looking at the code, this can be implemented too. Method pattern parsing code might benefit from some fancy delimiter, such as |
|
compiler/compilercontrol/commands/CompileLevelPrintTest.java failed: |
I suggest t todo it in separate RFE if you want to do this. |
Yes. And may be in I found this https://bugs.openjdk.org/browse/JDK-8308287 as example. |
It was an intermittent failure due to mixed up logging output from different VM places, but I was able to reproduce and fix it. I'm still running tests on my side, no such failures so far. |
I've added java.md changes here temporarily for review purposes (it should be another PR related to CSR) |
|
Text for I submitted new testing. |
Yes |
|
I still have compiler/compilercontrol/commands/CompileLevelPrintTest.java test failed: |
Implemented intx arguments in -XX:CompileCommand={break,compileonly,exclude,print} commands that allow to select specific compilation levels.
E.g.
-XX:CompileCommand=compileonly,java/lang/Object::wait,9would allow to compile the wait() method only on CompLevel 1 and 4 (1 << (4 - 1) | 1 << (1 - 1) == 9).For compatibility with the old format, specifying the commands without intx argument is also supported. In this case all levels will be covered (using a default value of 15). Specifying boolean values is not supported anymore.
New tests are provided and I ran them manually. The tests are not included into any tiers yet.
Some implementation choices I made (and I open to discussing/modifying them):
CompilationPolicy::select_task(), see line 817+CodeInstaller::install()method processes compilation level like this:DirectiveSet* directive = DirectivesStack::getMatchingDirective(method, compiler == nullptr || compiler->is_c1() ? CompLevel_simple : CompLevel_full_optimization);Please let me know if this needs changing.
C2V_VMENTRY_0(jboolean, hasNeverInlineDirective,(JNIEnv* env, jobject, ARGUMENT_PAIR(method))):return !Inline || CompilerOracle::should_not_inline(method, CompLevel_full_optimization) || method->dont_inline();(level 4 is assumed, please let me know if this has to be changed)
Verified by running 3 newly developed tests manually + tier1 tests on linux-x64, macosx-aarch64, windows-x64.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30352/head:pull/30352$ git checkout pull/30352Update a local copy of the PR:
$ git checkout pull/30352$ git pull https://git.openjdk.org/jdk.git pull/30352/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30352View PR using the GUI difftool:
$ git pr show -t 30352Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30352.diff
Using Webrev
Link to Webrev Comment