RFR(S): 8235673: [C1, C2] Split inlining control flags
Doerr, Martin
martin.doerr at sap.com
Mon May 11 13:32:31 UTC 2020
Hi Vladimir,
are you ok with the updated CSR (https://bugs.openjdk.java.net/browse/JDK-8244507)?
Should I set it to proposed?
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.
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.
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