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

[Doerr, Martin]

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.

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