RFR(S): 8235673: [C1, C2] Split inlining control flags
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed May 13 19:46:09 UTC 2020
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