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

[Patricio Chilano]

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
>>