[15] RFR(S): 8246203: Segmentation fault in verification due to stack overflow with -XX:+VerifyIterativeGVN

[Christian Hagedorn]

Thank you Vladimir and Tobias for having a look at it again and sharing 
your thoughts! Then I'll push webrev.01 into JDK 16 (as it's a P4 only).

Best regards,
Christian

On 12.06.20 14:02, Tobias Hartmann wrote:
> +1
> 
> Best regards,
> Tobias
> 
> On 11.06.20 18:27, Vladimir Kozlov wrote:
>> I would keep changes as they are - they provide correct testing. And treat timeouts as expected.
>>
>> Regards,
>> Vladimir
>>
>> On 6/11/20 12:38 AM, Christian Hagedorn wrote:
>>> Hi Tobias
>>>
>>> Thank you for your review!
>>>
>>> I ran some more testing with -XX:+VerifyIterativeGVN over night and compared the iterative
>>> solution with the old recursive version. We hit some more test timeouts with the new iterative
>>> solution because we are now really looking at all nodes for a requested depth. There are cases
>>> where the recursive solution would not do that. For example, if verify_depth = 4 and given a node
>>> chain 1->2->3->4->5, the recursive DFS solution visits nodes 1-4 and then at node 4 when it wants
>>> to visit node 5 it immediately returns because the depth 4 is reached. Node 4 is now marked as
>>> visited. If, however, there is an additional path 1->4->5->6->7 to look at later, the recursive
>>> DFS solution will just stop at node 4 because that node was already visited. The iterative
>>> solution, on the other hand, processes the nodes in a BFS and will visit all nodes up to depth 4,
>>> including node 5 and 6. This results in spending more time (as seen with more timeouts for more
>>> complex graphs/tests).
>>>
>>> We could now try to simulate the same recursive DFS behavior in an iterative approach with a
>>> stack. But as we seem to be missing some nodes at a requested depth this is probably not what we
>>> really want? Alternatively, we could go with this BFS solution as it is and decrement verify_depth
>>> = 4 in the call Node::verify(n, 4) to reduce the time spent. Or just leave webrev.01 as it is and
>>> treat the additional timeouts as expected.
>>>
>>> What do you think?
>>>
>>> Best regards,
>>> Christian
>>>
>>> On 10.06.20 14:30, Tobias Hartmann wrote:
>>>> Hi Christian,
>>>>
>>>> On 09.06.20 19:32, Christian Hagedorn wrote:
>>>>> http://cr.openjdk.java.net/~chagedorn/8246203/webrev.01/
>>>>
>>>> Looks good to me.
>>>>
>>>> Did you run testing with -XX:+VerifyIterativeGVN?
>>>>
>>>> Best regards,
>>>> Tobias
>>>>