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

[Alex Menkov]

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