[15] RFR(S): 8244724: CTW: C2 compilation fails with "Live Node limit exceeded limit"

[Vladimir Kozlov]

Good.

Thanks,
Vladimir

On 6/30/20 12:55 AM, Christian Hagedorn wrote:
> Hi Vladimir
> 
> On 29.06.20 20:22, Vladimir Kozlov wrote:
>> On 6/29/20 1:49 AM, Christian Hagedorn wrote:
>>> Thank you Nils and Vladimir for your reviews!
>>>
>>> On 27.06.20 00:35, Vladimir Kozlov wrote:
>>>> You don't need to use 'C->' in Compile::Optimize() method:
>>>>
>>>> DEBUG_ONLY(C->set_phase_optimize_finished();)
>>>
>>> Oh, right!
>>>
>>>> Yes, remove all nodes limit checks after optimization phase.
>>>
>>> I created a new webrev with all those Compile::check_node_count() calls removed and the change above:
>>> http://cr.openjdk.java.net/~chagedorn/8244724/webrev.02/
>>>
>>> However, since these calls also affect bailout decisions in the product version and this bug is targeted for 15, I 
>>> suggest to remove these calls/bailouts for 16 only in a separate RFE to minimize the risk.
>>
>> Agree. And please keep third failure check in chaitin.cpp because it also follows Split():
>>
>>   538     if (C->failing()) {
>>   539       return;
>>   540     }
>> > It seems okay to remove 1st and 4th checks as you did.
> 
> Right, that 3rd check should be kept.
> 
> Sounds good, then I'll push webrev.01 (without the bailouts being removed) together with this change:
>>>> You don't need to use 'C->' in Compile::Optimize() method:
>>>>
>>>> DEBUG_ONLY(C->set_phase_optimize_finished();)
> 
> I created a new RFE for 16 [1] to remove the bailouts as done additionally in webrev.02. I will run some testing and 
> then send out another review for [1].
> 
> Best regards,
> Christian
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8248529
> 
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Best regards,
>>> Christian
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 6/26/20 7:38 AM, Christian Hagedorn wrote:
>>>>> Hi
>>>>>
>>>>> Please review the following patch:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8244724
>>>>> http://cr.openjdk.java.net/~chagedorn/8244724/webrev.01/
>>>>>
>>>>> The testcase contains many string concatinations. These are compiled by javac with -XDstringConcat=inline which 
>>>>> creates a lot of StringBuilder objects and calls. As a result, we get a huge graph and eventually hit the live node 
>>>>> limit assert during code generation when trying to create new nodes - either during PhaseCFG::build_cfg() or later 
>>>>> in PhaseCFG::global_code_motion().
>>>>>
>>>>> We could try to introduce estimates for them to bailout but that appears to be difficult to get right without being 
>>>>> too pessimistic about it. But we need to be in order to avoid hitting the assert again by just modifying the testcase.
>>>>>
>>>>> Therefore, my suggestion is to completely skip the assert once the optimization phase is finished as we should not 
>>>>> strictly care about the node limit anymore at this point in time and it does not really provide much help for 
>>>>> finding bugs.
>>>>>
>>>>> A question remains, though, if we should also get rid of the remaining live node limit bailout checks in 
>>>>> Compile::Code_Gen() like [1] as it appears to be a waste to go through all the optimization in the optimization 
>>>>> phase to then bailout while generating code based only on the live node limit itself. What do you think about that?
>>>>>
>>>>> I also updated "<" into "<=" in the live node limit assert because we should be allowed to reach the limit but not 
>>>>> go beyond.
>>>>>
>>>>> Thank you!
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>>
>>>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/8fd3e34e8379/src/hotspot/share/opto/coalesce.cpp#l236