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

[serguei.spitsyn at oracle.com]

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