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

[Christian Hagedorn]

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 
>>>>