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

[Patricio Chilano]

Hi Dan,

On 7/26/19 3:19 PM, 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",
>
:-D

> 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.
Done! I added the bug number in the same line since that's the style I 
see in other tests.

> 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.
Done!

> Thumbs up. I don't need to see another webrev if you choose to make
> the above changes.
Great! Thanks for reviewing this Dan!  : )


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