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

[Patricio Chilano]

Hi Martin,

On 7/29/19 10:13 AM, Doerr, Martin wrote:
> Hi Patricio, I have also already noticed this issue. Thank you for 
> analyzing the root cause. Fix looks good to me. I don't need to see 
> another webrev for comment improvements, either. I've linked the bug 
> to JDK-8191890 and JDK-8219584. 
Great. I'll just add more comments for the other flags as David requested.

Thanks for reviewing this Martin!

Patricio
> Best regards, Martin
>> -----Original Message-----
>> From: hotspot-runtime-dev <hotspot-runtime-dev-
>> bounces at openjdk.java.net> On Behalf Of David Holmes
>> Sent: Samstag, 27. Juli 2019 00:28
>> To: daniel.daugherty at oracle.com; Patricio Chilano
>> <patricio.chilano.mateo at oracle.com>; hotspot-runtime-
>> dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: RFR 8227528: TestAbortVMOnSafepointTimeout.java failed due
>> to "RuntimeException: 'Safepoint sync time longer than' missing from
>> stdout/stderr"
>>
>> 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.ja
>> va
>>>       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