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

[Vladimir Kozlov]

nmethod has flags which record some compilation information:

   // set during construction
   unsigned int _has_unsafe_access:1;         // May fault due to unsafe 
access.
   unsigned int _has_method_handle_invokes:1; // Has this method 
MethodHandle invokes?
   unsigned int _lazy_critical_native:1;      // Lazy JNI critical native
   unsigned int _has_wide_vectors:1;          // Preserve wide vectors 
at safepoints

I thought about extending it. It could be difficult to justify adding a 
flag just for testing but if we add it only for debug VM it should be 
fine.  Do we do WB testing only with [fast]debug VM?

Anyway, I agree with your exception after loop - changes are fine now. 
Reviewed.

Thanks,
Vladimir

On 2/17/16 5:15 AM, Andreas Eriksson wrote:
> 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
>>>>
>>>
>