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

[Christian Hagedorn]

Thank you Vladimir for your review!

Best regards,
Christian

On 30.06.20 18:42, Vladimir Kozlov wrote:
> 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 
>>>>>>