+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.li...@oracle.com
<mailto: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
