8251462: Simplify compilation policy #1985
Conversation
veresov
changed the title
|
|
|
@veresov 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
veresov
changed the title
|
/label hotspot-compiler |
|
@veresov |
Webrevs
|
| * While running the thread, it calls a method (doTask2() ) for number of times (10000). | ||
| * That would cause this method to be compiled, which causes a jvmti callback for compiled method load. | ||
| * (Hint : to force method compilation -XX:CompileThreshold=900 is used). |
There was a problem hiding this comment.
The test still uses (and the comments state it uses) -XX:CompileThreshold=900 to force compilation, yet it looks like you needed to bump the number of times the method is called from 1000 to 10000 to keep this test working. Why doesn't 1000 still work?
There was a problem hiding this comment.
Tiered policy (which the -TieredCompilation emulation mode piggybacks on now) functions by doing periodic callbacks into the runtime to make policy decisions. The callback intervals are a power-of-2. So, because of the resulting rounding I cannot guarantee that the compilation will be triggered exactly at the specified threshold. Plus there are various adaptive mechanisms that I kept on even in the emulation mode that may also result in a method being compiled later than expected. To summarize, in the new world, the advisable way of triggering a compilation is to use the WB API. If that's too much effort, make sure that the method that you'd like to compile is invoked more than 2^(log2(CompileThreshold) + 1) times, plus a bit more. Very few tests actually depend on the legacy behavior, this just happens to be one of those.
|
@veresov This change now passes all automated pre-integration checks. 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 31 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.
|
|
Mailing list message from David Holmes on shenandoah-dev: Hi Igor, On 8/01/2021 6:36 am, Igor Veresov wrote:
Can you clarify, for non-compiler folk, what all the alternative configs define_pd_global(bool, TieredCompilation, as I expected tiered compilation to require COMPILER1 and COMPILER2. Also I see interpreter code that used to be guarded by TieredCompilation Overall it is a big change to digest, but after skimming it looks like a Thanks, |
|
Please find the answers in-line.
That's in a c2_globals_*.hpp, which is only included if C2 is present. Given that, if C1 is present as well the flag is set to true.
Right. That's exactly what's happening.
Counters and compilation policy callbacks are historically guarded by UseCompiler and UseLoopCounter flags, which are set to false if there are no compilers present. I haven't changed the semantics.
Frankly, I don't know how to split it into smaller pieces. There are surprisingly lots of interdependencies. However, the best way to think about it is that there are three major parts: 1. CompilerConfig API that is an abstraction over compiler configuration (what's compiled in, what flags are present that restrict compiler usage, etc); 2. The legacy policy removal. 3. Emulation of the old JVM behavior. As far as the runtime part of the change is concerted, it's pretty much only the legacy policy removal. In particular, the parts of the interpreter that dealt with the old policy (basically everything in the interpreter that was under !TieredCompilation has gotten removed). Igor
|
|
To clarify the possible configs.
|
|
Mailing list message from David Holmes on shenandoah-dev: On 8/01/2021 4:09 pm, Igor Veresov wrote:
Sorry I overlooked the exact placement.
That is not at all obvious. For example in 608: __ bind(no_mdo); but now it seems to be executed unconditionally with no reference to
I was thinking the ifdef related changes as one commit; then the These aren't separate changes just different commits within the PR so Cheers, |
|
Dave, I'm answering inline.
This code is in generate_counter_incr() each call to which is guarded by
I could've done that, I guess, but I usually like to make my checkpoints compilable. Sorry.
|
| __ cmp_32(Ricnt, R1_tmp); | ||
| __ b(*overflow, hs); | ||
| __ bind(done); | ||
| int increment = InvocationCounter::count_increment; |
There was a problem hiding this comment.
The small code below seems not to be equivalent replacement for the above code. I would appreciate some explanation on why we change the things. Thanks.
There was a problem hiding this comment.
The code that is currently there should be exactly the code that was guarded by if (TieredCompilation) before. The else part (corresponding to !TieredCompilation is what's gotten removed. Since the new policy is based off of tiered we don't need the old style of callbacks. Is there something else that you've noticed?
|
I see some regression on ARM32 with this change: |
I don't think those are related to the changes. Those are probably some preexisting c1 and c2 problems respectively that for some reason weren't triggered before. The TestOptionsWithRanges is my fault though. I'll fix that. |
|
I've added the flag validity pre-checks, which should solve the issues with VM complaining about tiered flags being invalid as a result of CompileThreshold and friends being invalid. We'd have to solve the C1 SIGILL and the C2 loop opts crash separately. Those are not really related to the change. |
|
Ok. I will see what is wrong there. |
src/hotspot/cpu/aarch64/gc/shenandoah/c1/shenandoahBarrierSetC1_aarch64.cpp
Outdated
Show resolved
Hide resolved
|
I asked Eric to run the benchmarks. The results should be ready on Wednesday. |
|
The benchmarking is done. No regressions. There are mild improvements in startup benchmarks on the order of 1-5% (I suspect because of the compilation policy devirtualization). @dholmes-ora, do still plan to look at this? It's been a few weeks, I'd like to push this. |
|
Please update from current master to get x86_32 built and tested in GH Actions. |
veresov
force-pushed
the
compilation-policy
branch
from
74ebbcb
to
44045ec
Compare
|
Mailing list message from David Holmes on shenandoah-dev: Hi Igor, On 28/01/2021 2:46 am, Igor Veresov wrote:
No I only perused this and made a couple of comments, now addressed. I'll leave a full review to others. Thanks, |
veresov
changed the title
|
/integrate |
openjdk
bot
added
integrated
|
@veresov Since your change was applied there have been 31 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 1519632. |
This change removes the legacy compilation policy and an emulation mode to the tiered policy to simulate the old behavior with
-XX:-TieredCompilation. The change removed a bunch of interpreter code, devirtualizes the compilation policy API, adds a consistent way to query compiler configuration with the newCompilerConfigAPI.I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and works with C1/C2-Graal/AOT being enabled/disabled.
Since there are platform-specific changes I would greatly appreciate some help from the maintainers of the specific ports to verify the build and run basic smoke tests. I've already tested x64 and aarch64. Thanks!
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1985/head:pull/1985$ git checkout pull/1985