RFR(S): 8235673: [C1, C2] Split inlining control flags
Doerr, Martin
martin.doerr at sap.com
Thu May 14 19:14:19 UTC 2020
Hi Vladimir,
> But we can use it in Test5091921.java. C1 compiles the test code with
> specified value before - lets keep it.
Ok. That makes sense for this test. Updated webrev in place.
> And this is not related to these changes but to have range(0, max_jint) for all
> these flags is questionable. I think
> nobody ran tests with 0 or max_jint values. Bunch of tests may simple
> timeout (which is understandable) but in worst
> case they may crash instead of graceful exit.
I was wondering about that, too. But I haven't changed that. The previously global flags already had this range.
I had also thought about guessing more reasonable values, but reasonable limits may depend on platform and future changes.
I don't think we can define ranges such that everything works great while we stay inside and also such that nobody will ever want greater values.
So I prefer keeping it this way unless somebody has a better proposal.
Thanks and best regards,
Martin
> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Sent: Mittwoch, 13. Mai 2020 23:34
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: RFR(S): 8235673: [C1, C2] Split inlining control flags
>
> On 5/13/20 1:10 PM, Doerr, Martin wrote:
> > Hi Vladimir,
> >
> > thanks for reviewing it.
> >
> >>> Should I set it to proposed?
> >>
> >> Yes.
> > I've set it to "Finalized". Hope this was correct.
> >
> >>> I've added the new C1 flags to the tests which should test C1 compiler as
> >> well.
> >>
> >> Good. Why not do the same for C1MaxInlineSize?
> > Looks like MaxInlineSize is only used by tests which test C2 specific things.
> So I think C1MaxInlineSize would be pointless.
> > In addition to that, the C2 values are probably not appropriate for C1 in
> some tests.
> > Would you like to have C1MaxInlineSize configured in some tests?
>
> You are right in cases when test switch off TieredCompilation and use only C2
> (Test6792161.java) or tests intrinsics.
>
> But we can use it in Test5091921.java. C1 compiles the test code with
> specified value before - lets keep it.
>
> And this is not related to these changes but to have range(0, max_jint) for all
> these flags is questionable. I think
> nobody ran tests with 0 or max_jint values. Bunch of tests may simple
> timeout (which is understandable) but in worst
> case they may crash instead of graceful exit.
>
> Thanks,
> Vladimir
>
> >
> > Best regards,
> > Martin
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> >> Sent: Mittwoch, 13. Mai 2020 21:46
> >> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> >> dev at openjdk.java.net
> >> Subject: Re: RFR(S): 8235673: [C1, C2] Split inlining control flags
> >>
> >> Hi Martin,
> >>
> >> On 5/11/20 6:32 AM, Doerr, Martin wrote:
> >>> Hi Vladimir,
> >>>
> >>> are you ok with the updated CSR
> >> (https://bugs.openjdk.java.net/browse/JDK-8244507)?
> >>> Should I set it to proposed?
> >>
> >> Yes.
> >>
> >>>
> >>> Here's a new webrev with obsoletion + expiration for C2 flags in
> ClientVM:
> >>> http://cr.openjdk.java.net/~mdoerr/8235673_C1_inlining/webrev.02/
> >>>
> >>> I've added the new C1 flags to the tests which should test C1 compiler as
> >> well.
> >>
> >> Good. Why not do the same for C1MaxInlineSize?
> >>
> >>> And I've added -XX:+IgnoreUnrecognizedVMOptions to all tests which
> set
> >> C2 flags. I think this is the best solution because it still allows running the
> tests
> >> with GraalVM compiler.
> >>
> >> Yes.
> >>
> >> Thanks,
> >> Vladimir
> >>
> >>>
> >>> Best regards,
> >>> Martin
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Doerr, Martin
> >>>> Sent: Freitag, 8. Mai 2020 23:07
> >>>> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-
> >>>> dev at openjdk.java.net
> >>>> Subject: RE: RFR(S): 8235673: [C1, C2] Split inlining control flags
> >>>>
> >>>> Hi Vladimir,
> >>>>
> >>>>> You need update your CSR - add information about this and above
> code
> >>>> change. Example:
> >>>>> https://bugs.openjdk.java.net/browse/JDK-8238840
> >>>> I've updated the CSR with obsolete and expired flags as in the example.
> >>>>
> >>>>> I would suggest to fix tests anyway (there are only few) because new
> >>>>> warning output could be unexpected.
> >>>> Ok. I'll prepare a webrev with fixed tests.
> >>>>
> >>>> Best regards,
> >>>> Martin
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> >>>>> Sent: Freitag, 8. Mai 2020 21:43
> >>>>> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> >>>>> dev at openjdk.java.net
> >>>>> Subject: Re: RFR(S): 8235673: [C1, C2] Split inlining control flags
> >>>>>
> >>>>> Hi Martin
> >>>>>
> >>>>> On 5/8/20 5:56 AM, Doerr, Martin wrote:
> >>>>>> Hi Vladimir,
> >>>>>>
> >>>>>> thanks a lot for looking at this, for finding the test issues and for
> >>>> reviewing
> >>>>> the CSR.
> >>>>>>
> >>>>>> For me, C2 is a fundamental part of the JVM. I would usually never
> >> build
> >>>>> without it ��
> >>>>>> (Except if we want to use C1 + GraalVM compiler only.)
> >>>>>
> >>>>> Yes it is one of cases.
> >>>>>
> >>>>>> But your right, --with-jvm-variants=client configuration should still be
> >>>>> supported.
> >>>>>
> >>>>> Yes.
> >>>>>
> >>>>>>
> >>>>>> We can fix it by making the flags as obsolete if C2 is not included:
> >>>>>> diff -r 5f5ed86d7883 src/hotspot/share/runtime/arguments.cpp
> >>>>>> --- a/src/hotspot/share/runtime/arguments.cpp Fri May 08 11:14:28
> >>>> 2020
> >>>>> +0200
> >>>>>> +++ b/src/hotspot/share/runtime/arguments.cpp Fri May 08
> 14:41:14
> >>>>> 2020 +0200
> >>>>>> @@ -562,6 +562,16 @@
> >>>>>> { "dup option", JDK_Version::jdk(9),
> >>>> JDK_Version::undefined(),
> >>>>> JDK_Version::undefined() },
> >>>>>> #endif
> >>>>>>
> >>>>>> +#ifndef COMPILER2
> >>>>>> + // These flags were generally available, but are C2 only, now.
> >>>>>> + { "MaxInlineLevel", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> + { "MaxRecursiveInlineLevel", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> + { "InlineSmallCode", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> + { "MaxInlineSize", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> + { "FreqInlineSize", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> + { "MaxTrivialSize", JDK_Version::undefined(),
> >>>>> JDK_Version::jdk(15), JDK_Version::undefined() },
> >>>>>> +#endif
> >>>>>> +
> >>>>>> { NULL, JDK_Version(0), JDK_Version(0) }
> >>>>>> };
> >>>>>
> >>>>> Right. I think you should do full process for these product flags
> >> deprecation
> >>>>> with obsoleting in JDK 16 for VM builds
> >>>>> which do not include C2. You need update your CSR - add information
> >>>> about
> >>>>> this and above code change. Example:
> >>>>>
> >>>>> https://bugs.openjdk.java.net/browse/JDK-8238840
> >>>>>
> >>>>>>
> >>>>>> This makes the VM accept the flags with warning:
> >>>>>> jdk/bin/java -XX:MaxInlineLevel=9 -version
> >>>>>> OpenJDK 64-Bit Client VM warning: Ignoring option MaxInlineLevel;
> >>>>> support was removed in 15.0
> >>>>>>
> >>>>>> If we do it this way, the only test which I think should get fixed is
> >>>>> ReservedStackTest.
> >>>>>> I think it should be sufficient to add -XX:C1MaxInlineLevel=2 in order
> to
> >>>>> preserve the inlining behavior.
> >>>>>>
> >>>>>> (TestStringIntrinsics2: C1 doesn't have String intrinsics anymore.
> >>>>> compiler/c2 tests: Also written to test C2 specific things.)
> >>>>>>
> >>>>>> What do you think?
> >>>>>
> >>>>> I would suggest to fix tests anyway (there are only few) because new
> >>>>> warning output could be unexpected.
> >>>>> And it will be future-proof when warning will be converted into error
> >>>>> (if/when C2 goes away).
> >>>>>
> >>>>> Thanks,
> >>>>> Vladimir
> >>>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Martin
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: hotspot-compiler-dev <hotspot-compiler-dev-
> >>>>>>> bounces at openjdk.java.net> On Behalf Of Vladimir Kozlov
> >>>>>>> Sent: Donnerstag, 7. Mai 2020 19:11
> >>>>>>> To: hotspot-compiler-dev at openjdk.java.net
> >>>>>>> Subject: Re: RFR(S): 8235673: [C1, C2] Split inlining control flags
> >>>>>>>
> >>>>>>> I would suggest to build VM without C2 and run tests.
> >>>>>>>
> >>>>>>> I grepped tests with these flags I found next tests where we need
> to
> >> fix
> >>>>>>> test's command (add
> >>>>>>> -XX:+IgnoreUnrecognizedVMOptions) or add @requires
> >>>>>>> vm.compiler2.enabled or duplicate test for C1 with corresponding
> C1
> >>>>>>> flags (by ussing additional @test block).
> >>>>>>>
> >>>>>>> runtime/ReservedStack/ReservedStackTest.java
> >>>>>>> compiler/intrinsics/string/TestStringIntrinsics2.java
> >>>>>>> compiler/c2/Test6792161.java
> >>>>>>> compiler/c2/Test5091921.java
> >>>>>>>
> >>>>>>> And there is issue with compiler/compilercontrol tests which use
> >>>>>>> InlineSmallCode and I am not sure how to handle:
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>
> >>
> http://hg.openjdk.java.net/jdk/jdk/file/55e9cb6b23ec/test/hotspot/jtreg/c
> >>>>>>> ompiler/compilercontrol/share/scenario/Command.java#l36
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Vladimir
> >>>>>>>
> >>>>>>> On 5/4/20 9:04 AM, Doerr, Martin wrote:
> >>>>>>>> Hi Nils,
> >>>>>>>>
> >>>>>>>> thank you for looking at this and sorry for the late reply.
> >>>>>>>>
> >>>>>>>> I've added MaxTrivialSize and also updated the issue accordingly.
> >>>> Makes
> >>>>>>> sense.
> >>>>>>>> Do you have more flags in mind?
> >>>>>>>>
> >>>>>>>> Moving the flags which are only used by C2 into c2_globals
> definitely
> >>>>> makes
> >>>>>>> sense.
> >>>>>>>>
> >>>>>>>> Done in webrev.01:
> >>>>>>>>
> >> http://cr.openjdk.java.net/~mdoerr/8235673_C1_inlining/webrev.01/
> >>>>>>>>
> >>>>>>>> Please take a look and let me know when my proposal is ready for
> a
> >>>> CSR.
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>> Martin
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: hotspot-compiler-dev <hotspot-compiler-dev-
> >>>>>>>>> bounces at openjdk.java.net> On Behalf Of Nils Eliasson
> >>>>>>>>> Sent: Dienstag, 28. April 2020 18:29
> >>>>>>>>> To: hotspot-compiler-dev at openjdk.java.net
> >>>>>>>>> Subject: Re: RFR(S): 8235673: [C1, C2] Split inlining control flags
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> Thanks for addressing this! This has been an annoyance for a
> long
> >>>> time.
> >>>>>>>>>
> >>>>>>>>> Have you though about including other flags - like
> MaxTrivialSize?
> >>>>>>>>> MaxInlineSize is tested against it.
> >>>>>>>>>
> >>>>>>>>> Also - you should move the flags that are now c2-only to
> >>>>> c2_globals.hpp.
> >>>>>>>>>
> >>>>>>>>> Best regards,
> >>>>>>>>> Nils Eliasson
> >>>>>>>>>
> >>>>>>>>> On 2020-04-27 15:06, Doerr, Martin wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> while tuning inlining parameters for C2 compiler with JDK-
> 8234863
> >>>> we
> >>>>>>> had
> >>>>>>>>> discussed impact on C1.
> >>>>>>>>>> I still think it's bad to share them between both compilers. We
> >> may
> >>>>> want
> >>>>>>> to
> >>>>>>>>> do further C2 tuning without negative impact on C1 in the future.
> >>>>>>>>>>
> >>>>>>>>>> C1 has issues with substantial inlining because of the lack of
> >>>>> uncommon
> >>>>>>>>> traps. When C1 inlines a lot, stack frames may get large and code
> >>>> cache
> >>>>>>> space
> >>>>>>>>> may get wasted for cold or even never executed code. The
> >> situation
> >>>>> gets
> >>>>>>>>> worse when many patching stubs get used for such code.
> >>>>>>>>>>
> >>>>>>>>>> I had opened the following issue:
> >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8235673
> >>>>>>>>>>
> >>>>>>>>>> And my initial proposal is here:
> >>>>>>>>>>
> >>>>>
> http://cr.openjdk.java.net/~mdoerr/8235673_C1_inlining/webrev.00/
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Part of my proposal is to add an additional flag which I called
> >>>>>>>>> C1InlineStackLimit to reduce stack utilization for C1 methods.
> >>>>>>>>>> I have a simple example which shows wasted stack space (java
> >>>>> example
> >>>>>>>>> TestStack at the end).
> >>>>>>>>>>
> >>>>>>>>>> It simply counts stack frames until a stack overflow occurs. With
> >> the
> >>>>>>> current
> >>>>>>>>> implementation, only 1283 frames fit on the stack because the
> >> never
> >>>>>>>>> executed method bogus_test with local variables gets inlined.
> >>>>>>>>>> Reduced C1InlineStackLimit avoids inlining of bogus_test and
> we
> >> get
> >>>>>>> 2310
> >>>>>>>>> frames until stack overflow. (I only used C1 for this example. Can
> >> be
> >>>>>>>>> reproduced as shown below.)
> >>>>>>>>>>
> >>>>>>>>>> I didn't notice any performance regression even with the
> >> aggressive
> >>>>>>> setting
> >>>>>>>>> of C1InlineStackLimit=5 with TieredCompilation.
> >>>>>>>>>>
> >>>>>>>>>> I know that I'll need a CSR for this change, but I'd like to get
> >>>> feedback
> >>>>> in
> >>>>>>>>> general and feedback about the flag names before creating a
> CSR.
> >>>>>>>>>> I'd also be glad about feedback regarding the performance
> >> impact.
> >>>>>>>>>>
> >>>>>>>>>> Best regards,
> >>>>>>>>>> Martin
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Command line:
> >>>>>>>>>> jdk/bin/java -XX:TieredStopAtLevel=1 -
> XX:C1InlineStackLimit=20 -
> >>>>>>>>> XX:C1MaxRecursiveInlineLevel=0 -Xss256k -Xbatch -
> >> XX:+PrintInlining
> >>>> -
> >>>>>>>>>
> >> XX:CompileCommand=compileonly,TestStack::triggerStackOverflow
> >>>>>>>>> TestStack
> >>>>>>>>>> CompileCommand: compileonly
> TestStack.triggerStackOverflow
> >>>>>>>>>> @ 8 TestStack::triggerStackOverflow (15
> bytes)
> >>>>>>> recursive
> >>>>>>>>> inlining too deep
> >>>>>>>>>> @ 11 TestStack::bogus_test (33 bytes) inline
> >>>>>>>>>> caught java.lang.StackOverflowError
> >>>>>>>>>> 1283 activations were on stack, sum = 0
> >>>>>>>>>>
> >>>>>>>>>> jdk/bin/java -XX:TieredStopAtLevel=1 -
> XX:C1InlineStackLimit=10 -
> >>>>>>>>> XX:C1MaxRecursiveInlineLevel=0 -Xss256k -Xbatch -
> >> XX:+PrintInlining
> >>>> -
> >>>>>>>>>
> >> XX:CompileCommand=compileonly,TestStack::triggerStackOverflow
> >>>>>>>>> TestStack
> >>>>>>>>>> CompileCommand: compileonly
> TestStack.triggerStackOverflow
> >>>>>>>>>> @ 8 TestStack::triggerStackOverflow (15
> bytes)
> >>>>>>> recursive
> >>>>>>>>> inlining too deep
> >>>>>>>>>> @ 11 TestStack::bogus_test (33 bytes) callee
> >> uses
> >>>>> too
> >>>>>>>>> much stack
> >>>>>>>>>> caught java.lang.StackOverflowError
> >>>>>>>>>> 2310 activations were on stack, sum = 0
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> TestStack.java:
> >>>>>>>>>> public class TestStack {
> >>>>>>>>>>
> >>>>>>>>>> static long cnt = 0,
> >>>>>>>>>> sum = 0;
> >>>>>>>>>>
> >>>>>>>>>> public static void bogus_test() {
> >>>>>>>>>> long c1 = 1, c2 = 2, c3 = 3, c4 = 4;
> >>>>>>>>>> sum += c1 + c2 + c3 + c4;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> public static void triggerStackOverflow() {
> >>>>>>>>>> cnt++;
> >>>>>>>>>> triggerStackOverflow();
> >>>>>>>>>> bogus_test();
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> public static void main(String args[]) {
> >>>>>>>>>> try {
> >>>>>>>>>> triggerStackOverflow();
> >>>>>>>>>> } catch (StackOverflowError e) {
> >>>>>>>>>> System.out.println("caught " + e);
> >>>>>>>>>> }
> >>>>>>>>>> System.out.println(cnt + " activations were on stack, sum
> = "
> >> +
> >>>>>>> sum);
> >>>>>>>>>> }
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>
More information about the hotspot-compiler-dev
mailing list