On Mon, 11 Jan 2021 02:54:02 GMT, Igor Veresov <ivere...@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.hol...@oracle.com) on >> [shenandoah-dev](mailto:shenandoah-...@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
