RFR: 8251462: Remove legacy compilation policy [v2]

[Boris Ulasevich]

On Mon, 11 Jan 2021 02:54:02 GMT, Igor Veresov <iveresov at openjdk.org> wrote:

>> To clarify the possible configs.
>> 1. There is only one policy now. Functions with both compilers or a single compiler.
>> 2. The best way to control the operation mode is with the ```-XX:CompilationMode=``` flag. Possible values so far are: normal (aka default), high-only (c2 or graal only), quick-only(c1 only), high-only-quick-internal (a special mode for jar graal where only graal itself is compiled by c1, but the user code is compiled with graal). 
>> 3. There is a mode that emulates the behavior when the user specifies -TieredCompilation. That's basically the high-only mode.
>> 4. There is also support for legacy policy flags such as CompileThreshold, which would properly setup the policy parameters to match the old behavior.
>
> Dave, I'm answering inline.
> 
>> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [shenandoah-dev](mailto:shenandoah-dev at openjdk.java.net):_
>> 
>> On 8/01/2021 4:09 pm, Igor Veresov wrote:
>> 
>> > On Thu, 7 Jan 2021 21:49:56 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:
>> > > > Igor Veresov has updated the pull request incrementally with one additional commit since the last revision:
>> > > > Fix s390 build
>> > > 
>> > > 
>> > > Marking as reviewed, but only for the jvmti tests. I don't believe there are any other svc changes in this PR.
>> > 
>> > 
>> > Please find the answers in-line.
>> > > _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [shenandoah-dev](mailto:shenandoah-dev at openjdk.java.net):_
>> > > Hi Igor,
>> > > On 8/01/2021 6:36 am, Igor Veresov wrote:
>> > > > 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 new ```CompilerConfig``` API.
>> > > 
>> > > 
>> > > Can you clarify, for non-compiler folk, what all the alternative configs
>> > > actually mean now. I'm a bit confused by this definition:
>> > > define_pd_global(bool, TieredCompilation,
>> > > COMPILER1_PRESENT(true) NOT_COMPILER1(false));
>> > 
>> > 
>> > 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.
>> 
>> Sorry I overlooked the exact placement.
>> 
>> > > as I expected tiered compilation to require COMPILER1 and COMPILER2.
>> > 
>> > 
>> > Right. That's exactly what's happening.
>> > > Also I see interpreter code that used to be guarded by TieredCompilation
>> > > now being executed unconditionally, which seems to assume C1 or C2 must
>> > > be present?
>> > 
>> > 
>> > 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.
>> 
>> That is not at all obvious. For example in
>> templateInterpreterGenerator_aarch64.cpp this code used to guarded by
>> TieredCompilation:
>> 
>> 608: __ bind(no_mdo);
>> // Increment counter in MethodCounters
>> const Address invocation_counter(rscratch2,
>> MethodCounters::invocation_counter_offset() +
>> InvocationCounter::counter_offset());
>> __ get_method_counters(rmethod, rscratch2, done);
>> const Address mask(rscratch2,
>> in_bytes(MethodCounters::invoke_mask_offset()));
>> __ increment_mask_and_jump(invocation_counter, increment, mask,
>> rscratch1, r1, false, Assembler::EQ, overflow);
>> __ bind(done);
> 
> This code is in generate_counter_incr() each call to which is guarded by ```if (inc_counter)```, and ```inc_counter``` is defined as ``` bool inc_counter  = UseCompiler || CountCompiledCalls || LogTouchedMethods;```.  Again, I haven't changed that at all, that how it always worked. We may try to tidy this up but I feel like this change is big enough already.
>  
>> 
>> but now it seems to be executed unconditionally with no reference to
>> either flag you mentioned.
>> 
>> > > Overall it is a big change to digest, but after skimming it looks like a
>> > > few of the refactorings could have been applied in a layered fashion
>> > > using multiple commits to make it easier to review.
>> > 
>> > 
>> > 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.
>> 
>> I was thinking the ifdef related changes as one commit; then the
>> TieredCompilation removals; then the addition of CompilerConfig API ...
>> 
>> These aren't separate changes just different commits within the PR so
>> that it can be reviewed in stages.
> 
> I could've done that, I guess, but I usually like to make my checkpoints compilable. Sorry.
> 
>> 
>> Cheers,
>> David
>> -----

I see some regression on ARM32 with this change:
http://cr.openjdk.java.net/~bulasevich/tmp/8251462_jtreg_hotspot/

-------------

PR: https://git.openjdk.java.net/jdk/pull/1985