RFR(S): 8235673: [C1, C2] Split inlining control flags
Doerr, Martin
martin.doerr at sap.com
Fri May 15 11:41:30 UTC 2020
Hi Vladimir, Nils and Tobias,
Can I consider http://cr.openjdk.java.net/~mdoerr/8235673_C1_inlining/webrev.02/ reviewed by you?
Submission repo testing was successful.
Thanks and best regards,
Martin
> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Sent: Donnerstag, 14. Mai 2020 22:29
> 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/14/20 12:14 PM, Doerr, Martin wrote:
> > 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.
>
> Good.
>
> >
> >> 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.
>
> I did not mean to have that in these change. Current changes are fine for me.
>
> I was thinking aloud that it would be nice to investigate this later by
> someone. At least for some flags. We may keep
> current range as it is but may be add dynamic checks based on platform and
> other conditions. This looks like starter
> task for junior engineer or student intern.
>
> Thanks,
> Vladimir
>
> >
> > 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