RFR(S): 8235673: [C1, C2] Split inlining control flags
Doerr, Martin
martin.doerr at sap.com
Wed May 13 20:10:40 UTC 2020
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?
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