RFR 8227528: TestAbortVMOnSafepointTimeout.java failed due to "RuntimeException: 'Safepoint sync time longer than' missing from stdout/stderr"

[David Holmes]

Looks good!

Thanks,
David

On 30/07/2019 9:45 am, Patricio Chilano wrote:
> Hi David,
> 
> On 7/26/19 6:27 PM, David Holmes wrote:
>> On 27/07/2019 5:19 am, Daniel D. Daugherty wrote:
>>> On 7/26/19 2:46 PM, Patricio Chilano wrote:
>>>> Hi all,
>>>>
>>>> Could you review this small fix for test 
>>>> TestAbortVMOnSafepointTimeout.java?
>>>>
>>>> The test has been failing intermittently since 8191890. As explained 
>>>> in the bug comments, it turns out that a bias revocation handshake 
>>>> could happen in between the start of the "for" loop without 
>>>> safepoint polls and the safepoint where we want to timeout. That 
>>>> allows for the long loop to actually finish and prevents the desired 
>>>> timeout in the later safepoint. The simple solution is to just avoid 
>>>> using biased locking in this test (and therefore prevent the 
>>>> revocation handshake), since we just want to test the correct 
>>>> behavior of flag AbortVMOnSafepointTimeout.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8227528/v01/webrev
>>>
>>> The change itself is trivial. However, the reasons behind the change 
>>> aren't.
>>>
>>> This part of the description caught my eye:
>>>
>>>      the start of the "for" loop without safepoint polls
>>>
>>> and my brain did a "Say what?!?!" Of course, that was without looking at
>>> the test which has a huge number of options, including these:
>>>
>>>      L70:                 "-XX:-UseCountedLoopSafepoints",
>>>      L71:                 "-XX:LoopStripMiningIter=0",
>>>      L72:                 "-XX:LoopUnrollLimit=0",
>>>
>>> Okay, now the world makes much more sense. We are intentionally telling
>>> the compiler to not emit safepoint polls in the counted loop and we're
>>> turning off other loop optimizations. Basically, we're telling the
>>> compiler we want to stall in that loop until we exceed the safepoint
>>> timeout limit. Got it...
>>>
>>> So the new biased locking handshake messes with the timeout that this
>>> test is trying to achieve. Disabling biased locking makes the test more
>>> robust by allowing the safepoint sync timeout to happen.
>>>
>>> A couple of minor suggestions:
>>>
>>> test/hotspot/jtreg/runtime/Safepoint/TestAbortVMOnSafepointTimeout.java
>>>      L30:  * @bug 8219584
>>>
>>>          You should add an @bug for this bug (8227528). I don't know if
>>>          you can put more than one bug ID on an @bug line or if you need
>>>          a separate @bug line.
>>>
>>>      L61:         ProcessBuilder pb = 
>>> ProcessTools.createJavaProcessBuilder(
>>>          Please add a comment above this line:
>>>
>>>              // -XX:-UseBiasedLocking - is used to prevent biased 
>>> locking
>>>              // handshakes from changing the timing of this test.
>>>
>>> Thumbs up. I don't need to see another webrev if you choose to make
>>> the above changes.
>>
>> I think some additional commentary on the other exotic options to 
>> ensure the loop contains no safepoints and is not unrolled etc would 
>> also be worthwhile.
> I added comments for flags UseCountedLoopSafepoints, LoopStripMiningIter 
> and LoopUnrollLimit. Here are the links to v02:
> 
> Full: http://cr.openjdk.java.net/~pchilanomate/8227528/v02/webrev/
> Inc: http://cr.openjdk.java.net/~pchilanomate/8227528/v02/inc/webrev/ 
> <http://cr.openjdk.java.net/~pchilanomate/8227528/v02/webrev/>
> 
> Thanks for looking at this David!
> 
> 
> Patricio
>> Change itself makes sense.
>>
>> Thanks,
>> David
>>
>>>
>>> Dan
>>>
>>>
>>>> Bugid: https://bugs.openjdk.java.net/browse/JDK-8227528
>>>>
>>>> Thanks!
>>>> Patricio
>>>
>