RFR(S): 8146096: [TEST BUG] compiler/loopopts/UseCountedLoopSafepoints.java Timeouts

[Andreas Eriksson]

Hi,

On 2016-02-16 20:45, Vladimir Kozlov wrote:
> So you implemented Vladimir Ivanov's suggestion.

Yes, and also added whitebox API calls to make sure that the method is 
C2 compiled, so checking the call stack shouldn't be necessary.

Regarding your suggestion on a new WB interface:
I wasn't sure on how to do use WB to check that a SafePointNode was 
generated. It looks to me like WB isn't hooked in to the actual 
compilation steps, but instead IsMethodCompiled etc. just lookup fields 
on the nmethod, at a point where we don't have access to the Ideal graph 
anymore. So adding an interface to actually hook into the Ideal graph to 
check for a SafePointNode seemed like a big investment for this test.
If I've misunderstood what you meant, or if there actually is an easy 
way to do this, please let me know.

> Okay but you should add a check that it did not finish the loop by 
> checking _num value. Imaging very fast machine that will take less 
> then 2 sec to finish loop without safepoint.

Changed it so it throws an exception if the loop is ever finished.
New webrev: http://cr.openjdk.java.net/~aeriksso/8146096/webrev.01/
Diff: http://cr.openjdk.java.net/~aeriksso/8146096/webrev.00.to.01/

Regards,
Andreas

>
> Thanks,
> Vladimir
>
> On 2/16/16 6:24 AM, Andreas Eriksson wrote:
>> Hi,
>>
>> I've changed the test so it will not timeout, by using the
>> WhiteBox.forceSafepoint() in a separate thread.
>> If there is a safepoint in the loop, then the test will exit after
>> taking one safepoint.
>> If instead all safepoints in the loop were removed, then the safepoint
>> operation will timeout after two seconds (because of
>> -XX:+SafepointTimeout) and it will be detected and the test will fail.
>>
>> Webrev: http://cr.openjdk.java.net/~aeriksso/8146096/webrev.00/
>>
>> Regards,
>> Andreas
>>
>> On 2016-01-20 10:26, Andreas Eriksson wrote:
>>> Vladimir Kozlov and Vladimir Ivanov,
>>>
>>> Ok, I'll look into using the whitebox api to fix the test.
>>> Thanks for looking at this.
>>>
>>> - Andreas
>>>
>>> On 2016-01-19 21:46, Vladimir Kozlov wrote:
>>>> Simple use timeout to check for generated safepoint is bad idea. It
>>>> is very inaccurate. At least you need to check call stack to see if
>>>> it stopped in compiled method.
>>>> I would prefer to see WB new interface which would check that loop
>>>> SafePointNode is generated during compilation of method. It will be
>>>> precise.
>>>>
>>>> And we need such tests to make sure a feature is working - we can't
>>>> remove them.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 1/19/16 4:50 AM, Vladimir Ivanov wrote:
>>>>> As an idea to improve the test: spawn a thread which executes the
>>>>> counted loop and then use WhiteBox.forceSafepoint() to
>>>>> trigger a safepoint.
>>>>>
>>>>> If the test times out, it means there's no safepoint in the loop.
>>>>>
>>>>> Also, it also simplifies the implementation - no need to spawn a
>>>>> child process, the check can be done in-process.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> On 1/19/16 3:32 PM, Andreas Eriksson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Can I please have a review for the removal of
>>>>>> hotspot/test/compiler/loopopts/UseCountedLoopSafepoints.java.
>>>>>>
>>>>>> The test needs to do a loop that takes more than two seconds to
>>>>>> execute
>>>>>> fully without doing a safepointing call. For this expensive atomic
>>>>>> operations were used. The problem is that on certain embedded
>>>>>> platforms
>>>>>> they are too expensive, and the test times out.
>>>>>> The loop length could probably be reduced, and it should still 
>>>>>> work on
>>>>>> faster machines. However, the test is not very useful, so I think 
>>>>>> it's
>>>>>> better to just remove it to avoid future problems.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8146096
>>>>>> Test to be removed:
>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/d84a55e7aaf8/test/compiler/loopopts/UseCountedLoopSafepoints.java 
>>>>>>
>>>>>>
>>>>>>
>>>>>> (I can create a webrev if you think it necessary.)
>>>>>>
>>>>>> Thanks,
>>>>>> Andreas
>>>
>>