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.li...@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
