[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S) 8207765: HeapMonitorIntervalRateTest fails with ZGC
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-07-20 1:22:49
Message-ID: CAF9BGBzSFR=Lr8SCCncQAMBres_yHtHs9cjs+nVfhTLOb3mwjQ () mail ! gmail ! com
[Download RAW message or body]
Hi Serguei and Alexey,
Thanks both and here you are:
http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.02/
Let me know if you need anything else!
Jc
On Thu, Jul 19, 2018 at 5:21 PM serguei.spitsyn@oracle.com <
serguei.spitsyn@oracle.com> wrote:
> Thanks a lot, Alex!
>
> Jc,
>
> Could you please send me a patch for push?
>
> Thanks,
> Serguei
>
> On 7/19/18 17:06, Alex Menkov wrote:
> > Looks good.
> >
> > --alex
> >
> > On 07/19/2018 14:52, JC Beyler wrote:
> > > Hi Serguei,
> > >
> > > Done here:
> > > http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.01/
> > > <http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.01/>
> > >
> > > I added:
> > >
> > > + // Calculate the size of a 1-element array in order to assess
> > > average sampling interval
> > > + // via the HeapMonitorStatIntervalTest. This is needed because
> > > various GCs could add
> > > + // extra memory to arrays.
> > > + // This is done by allocating a 1-element array and then looking in
> > > the heap monitoring
> > > + // samples for the average size of objects collected.
> > >
> > >
> > > Let me know what you think and then I need one more review to prepare
> > > the patch :-)
> > >
> > > Thanks all!
> > > Jc
> > >
> > > On Thu, Jul 19, 2018 at 2:33 PM serguei.spitsyn@oracle.com
> > > <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn@oracle.com
> > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > >
> > > Hi Jc,
> > >
> > > The fix looks good to me.
> > > Just minor comments.
> > >
> > >
> http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java.frames.html
>
> > >
> > > 108 public static void calculateAverageOneElementSize() {
> > >
> > > Could you, please, add a comment before
> > > calculateAverageOneElementSize method
> > > explaining shortly why it is needed and what it is doing?
> > > Otherwise, it is not easy to understand this code from scratch.
> > >
> > > Thanks,
> > > Serguei
> > >
> > >
> > > On 7/19/18 10:08, JC Beyler wrote:
> > > > I forgot to put the link:
> > > > https://bugs.openjdk.java.net/browse/JDK-8207763
> > > >
> > > > It got renamed in jdk11 via:
> > > > http://hg.openjdk.java.net/jdk/jdk11/rev/1edcf36fe15f
> > > >
> > > > Thanks!
> > > > Jc
> > > >
> > > > On Thu, Jul 19, 2018 at 10:07 AM JC Beyler <jcbeyler@google.com
> > > > <mailto:jcbeyler@google.com>> wrote:
> > > >
> > > > Hi Dan,
> > > >
> > > >
> > > > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatRateTest.java
> > > > became
> > > >
> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java,
> > > > when we updated the spec and said "rate" was the wrong word.
> > > >
> > > > So yes, it fixes both since at some point all branches should
> > > > see that the StatRate test becomes renamed into the
> > > > StatInterval test. Does that make sense?
> > > >
> > > > Thanks!
> > > > Jc
> > > >
> > > >
> > > > On Thu, Jul 19, 2018 at 9:45 AM Daniel D. Daugherty
> > > > <daniel.daugherty@oracle.com
> > > > <mailto:daniel.daugherty@oracle.com>> wrote:
> > > >
> > > > JDK-8207765 covers two different tests as of yesterday:
> > > >
> > > > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatRateTest.java
> > > >
> > > > and
> > > >
> > > >
> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> > > >
> > > > I updated it to add a similar failure mode sighting for
> > > > HeapMonitorStatIntervalTest.java
> > > >
> > > >
> > > > Does your fix address both test failures?
> > > >
> > > > Dan
> > > >
> > > >
> > > > On 7/19/18 12:39 PM, JC Beyler wrote:
> > > > > Hi all,
> > > > >
> > > > > Could I have a few reviews of:
> > > > > http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/
> > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/>
> > > > >
> > > > > The test assumed the size of a 1-element array but ZGC
> > > > > changes that assumption. The test now first allocates a
> > > > > bit of memory and gets the average size of the samples
> > > > > before assuming the size. This works with/without ZGC.
> > > > >
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/
> > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/>
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8207765
> > > > >
> > > > > Thanks!
> > > > > Jc
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Jc
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Jc
> > >
> > >
> > >
> > > --
> > >
> > > Thanks,
> > > Jc
>
>
--
Thanks,
Jc
[Attachment #3 (text/html)]
<div dir="ltr">Hi Serguei and Alexey,<div><br></div><div>Thanks both and here you \
are:</div><div><a href="http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.02/">http: \
//cr.openjdk.java.net/~jcbeyler/8207765/webrev.02/</a><br></div><div><br></div><div>Let \
me know if you need anything else!</div><div>Jc</div></div><br><div \
class="gmail_quote"><div dir="ltr">On Thu, Jul 19, 2018 at 5:21 PM <a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <<a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks a lot, Alex!<br> <br>
Jc,<br>
<br>
Could you please send me a patch for push?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 7/19/18 17:06, Alex Menkov wrote:<br>
> Looks good.<br>
><br>
> --alex<br>
><br>
> On 07/19/2018 14:52, JC Beyler wrote:<br>
>> Hi Serguei,<br>
>><br>
>> Done here:<br>
>> <a href="http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.01/</a> \
<br> >> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.01/</a>><br>
>><br>
>> I added:<br>
>><br>
>> + // Calculate the size of a 1-element array in order to assess <br>
>> average sampling interval<br>
>> + // via the HeapMonitorStatIntervalTest. This is needed because <br>
>> various GCs could add<br>
>> + // extra memory to arrays.<br>
>> + // This is done by allocating a 1-element array and then looking in <br>
>> the heap monitoring<br>
>> + // samples for the average size of objects collected.<br>
>><br>
>><br>
>> Let me know what you think and then I need one more review to prepare <br>
>> the patch :-)<br>
>><br>
>> Thanks all!<br>
>> Jc<br>
>><br>
>> On Thu, Jul 19, 2018 at 2:33 PM <a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> <br> >> <mailto:<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>> <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> <br> >> <mailto:<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>>> wrote:<br> >><br>
>> Hi Jc,<br>
>><br>
>> The fix looks good to me.<br>
>> Just minor comments.<br>
>><br>
>> <a href="http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java.frames.html" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev \
.00/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java.frames.html</a><br>
>><br>
>> 108 public static void calculateAverageOneElementSize() {<br>
>><br>
>> Could you, please, add a comment before<br>
>> calculateAverageOneElementSize method<br>
>> explaining shortly why it is needed and what it is doing?<br>
>> Otherwise, it is not easy to understand this code from \
scratch.<br> >><br>
>> Thanks,<br>
>> Serguei<br>
>><br>
>><br>
>> On 7/19/18 10:08, JC Beyler wrote:<br>
>>> I forgot to put the link:<br>
>>> <a href="https://bugs.openjdk.java.net/browse/JDK-8207763" \
rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8207763</a><br>
>>><br>
>>> It got renamed in jdk11 via:<br>
>>> <a href="http://hg.openjdk.java.net/jdk/jdk11/rev/1edcf36fe15f" \
rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk/jdk11/rev/1edcf36fe15f</a><br>
>>><br>
>>> Thanks!<br>
>>> Jc<br>
>>><br>
>>> On Thu, Jul 19, 2018 at 10:07 AM JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a><br> \
>>> <mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a>>> wrote:<br> >>><br>
>>> Hi Dan,<br>
>>><br>
>>><br>
>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatRateTest.java<br>
>>> became<br>
>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java,<br>
>>> when we updated the spec and said "rate" was \
the wrong word.<br> >>><br>
>>> So yes, it fixes both since at some point all branches \
should<br> >>> see that the StatRate test becomes renamed \
into the<br> >>> StatInterval test. Does that make sense?<br>
>>><br>
>>> Thanks!<br>
>>> Jc<br>
>>><br>
>>><br>
>>> On Thu, Jul 19, 2018 at 9:45 AM Daniel D. Daugherty<br>
>>> <<a href="mailto:daniel.daugherty@oracle.com" \
target="_blank">daniel.daugherty@oracle.com</a><br> >>> \
<mailto:<a href="mailto:daniel.daugherty@oracle.com" \
target="_blank">daniel.daugherty@oracle.com</a>>> wrote:<br> >>><br>
>>> JDK-8207765 covers two different tests as of \
yesterday:<br> >>><br>
>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatRateTest.java<br>
>>><br>
>>> and<br>
>>><br>
>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java<br>
>>><br>
>>> I updated it to add a similar failure mode \
sighting for<br> >>> \
HeapMonitorStatIntervalTest.java<br> >>><br>
>>><br>
>>> Does your fix address both test failures?<br>
>>><br>
>>> Dan<br>
>>><br>
>>><br>
>>> On 7/19/18 12:39 PM, JC Beyler wrote:<br>
>>>> Hi all,<br>
>>>><br>
>>>> Could I have a few reviews of:<br>
>>>> <a href="http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/</a><br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/</a>><br> \
>>>><br> >>>> The test assumed the \
size of a 1-element array but ZGC<br> >>>> changes \
that assumption. The test now first allocates a<br> >>>> \
bit of memory and gets the average size of the samples<br> >>>> \
before assuming the size. This works with/without ZGC.<br> >>>><br>
>>>> Webrev:<br>
>>>> <a href="http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8207765/webrev.00/</a><br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8207765/webrev.00/</a>><br> \
>>>> Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8207765" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8207765</a><br> \
>>>><br> >>>> Thanks!<br>
>>>> Jc<br>
>>><br>
>>><br>
>>><br>
>>> --<br>
>>> Thanks,<br>
>>> Jc<br>
>>><br>
>>><br>
>>><br>
>>> --<br>
>>> Thanks,<br>
>>> Jc<br>
>><br>
>><br>
>><br>
>> -- <br>
>><br>
>> Thanks,<br>
>> Jc<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic