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

[David Holmes]

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.

Change itself makes sense.

Thanks,
David

> 
> Dan
> 
> 
>> Bugid: https://bugs.openjdk.java.net/browse/JDK-8227528
>>
>> Thanks!
>> Patricio
>