On Fri, 8 Jan 2021 06:29:10 GMT, Igor Veresov <ivere...@openjdk.org> wrote:
>> Please find the answers in-line. >> >>> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) >>> on [shenandoah-dev](mailto:shenandoah-...@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. >>> >>> 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. >> >>> 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. >> >> 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 >> >>> Thanks, >>> David >>> ----- > > 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 > ----- ------------- PR: https://git.openjdk.java.net/jdk/pull/1985
