RFR (XS) 8215329: Modify ZGC requirement for HeapMonitorThreadTest.java

[JC Beyler]

Hi Per,

Thanks for the messages and review :-). I believe that actually what
happened was that when JDK11 was close to release both ZGC and
HeapMonitoring tried to get in. In a last effort, we turned off this test
for ZGC as it was showing test failures for ZGC. It's a bit fuzzy to be
honest (I was also on paternity leave and Serguei & Jeremy were helping out
here). But anyway, what seems to be true now is that you found a bug (yeah
I guess?) and we can remove the @requires once you fix your race.

Therefore, could I please get another review to update the @requires to be
correct then until then?

(@Per: if you want, I can update the test once it's done; either assign a
JBS bug to me or send me an email)

Thanks,
Jc

On Thu, Dec 13, 2018 at 4:47 AM Per Liden <per.liden at oracle.com> wrote:

> Hi, me again ;)
>
> I think I've found the root cause of this. There's a tiny race in the
> ZGC allocation path, which can lead to pre-mature OOME being thrown.
> It's not trivial to fix, so I suggest you go ahead with your original
> patch (Looks good btw), and I'll file a separate bug to fix the ZGC
> issue (and update this test to run with ZGC again).
>
> cheers,
> Per
>
> On 12/13/2018 12:21 PM, Per Liden wrote:
> > Hi again,
> >
> > I ran this test some more and managed to get an OOME even with a 768M
> > heap. I'm getting a bit suspicious that something else is wrong here.
> > Let me dig into this some more and see if I can understand what the real
> > issue is.
> >
> > cheers,
> > Per
> >
> > On 12/13/2018 10:31 AM, Per Liden wrote:
> >> Hi JC,
> >>
> >> What's the reason to exclude ZGC from this test to begin with? From
> >> what I can tell, it's because the test is using a slightly too small
> >> heap, or are there some other reason? I ran it a few times using
> >> various heap sizes and the test passes with ZGC when using anything
> >> above 612M. So if we instead just dump the heap size a bit, then we
> >> get test coverage with ZGC too. I picked 768M here to have some
> >> headroom in case the exact limit is run-to-run dependent.
> >>
> >> diff --git
> >>
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java
>
> >>
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java
>
> >>
> >> ---
> >>
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java
>
> >>
> >> +++
> >>
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java
>
> >>
> >> @@ -29,8 +29,7 @@
> >>    * @build Frame HeapMonitor ThreadInformation
> >>    * @summary Verifies the JVMTI Heap Monitor Thread information sanity.
> >>    * @compile HeapMonitorThreadTest.java
> >> - * @run main/othervm/native -Xmx512m -agentlib:HeapMonitorTest
> >> MyPackage.HeapMonitorThreadTest
> >> - * @requires !vm.gc.Z
> >> + * @run main/othervm/native -Xmx768m -agentlib:HeapMonitorTest
> >> MyPackage.HeapMonitorThreadTest
> >>    */
> >>
> >>   import java.util.List;
> >>
> >> cheers,
> >> Per
> >>
> >> On 12/13/2018 05:44 AM, JC Beyler wrote:
> >>> Hi all,
> >>>
> >>> When working on another webrev, I saw this problem:
> >>>
> >>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215329/webrev.00/
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8215329
> >>>
> >>> (Basically, from what I understood from an email from Per Liden:
> >>>     - @requires !vm.gc.Z -> ZGC is built in the JDK
> >>>     - @requires vm.gc != "Z" -> ZGC is being used for the runtime
> >>> )
> >>>
> >>> Thanks,
> >>> Jc
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181213/a2835dd0/attachment.html>