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

[Per Liden]

Just a follow up. After some further debugging, the real cause for this 
issue turned out to be a bug in JNI IsSameObject(). Fix currently out 
for review on hotspot-runtime-dev.

https://bugs.openjdk.java.net/browse/JDK-8215451

cheers,
Per

On 12/14/2018 07:52 PM, JC Beyler wrote:
> Thanks all, tested & pushed.
> 
> @Per: let me know when you resolve your bug and if you want me to do 
> anything from my side
> 
> On Thu, Dec 13, 2018 at 10:25 AM serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com>> wrote:
> 
>     +1
> 
>     Thanks,
>     Serguei
> 
> 
>     On 12/13/18 09:40, Alex Menkov wrote:
>      > +1
>      >
>      > --alex
>      >
>      > On 12/13/2018 08:59, JC Beyler wrote:
>      >> 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
>     <mailto:per.liden at oracle.com>
>      >> <mailto:per.liden at oracle.com <mailto: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
> 
> 
> 
> -- 
> 
> Thanks,
> Jc