RFR(S) 8207765: HeapMonitorIntervalRateTest fails with ZGC

Wed Aug 8 18:07:42 UTC 2018

Hi Per,

You are correct, my test shows that an int[1] will use 32 bytes
with -XX:-UseCompressedClassPointers under G1. My apologies for not digging
more into why with ZGC :-)

In any case, my test was fragile since using that flag would break it, the
fix was not a ZGC only fix, I basically made it first calculate the size
and then use that for the test. So, it is at least a better test and should
be stable now (famous last words).

Thanks for pointing out the actual reason why there were 8 extra bytes!
Jc

On Wed, Aug 8, 2018 at 6:19 AM Per Liden <per.liden at oracle.com> wrote:

> Hi JC,
>
> (Sorry, I didn't see this thread until now... been on vacation, jvmls, etc)
>
> On 07/19/2018 11:52 PM, JC Beyler wrote:
> > Hi Serguei,
> >
> > Done here:
> > http://cr.openjdk.java.net/~jcbeyler/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.
> >
> >
>
> I was just a bit curious as to what the actual problem was here. While
> it's true that a GC might use extra memory for an object, ZGC doesn't do
> that. An int[1] in ZGC has the same size as in G1/Parallel/Serial/CMS.
> The only exception I can think of is that you where using compressed
> class pointers, which is generally enabled by default, but
> disabled/unsupported in ZGC. Given this, I'm thinking you should have
> seen the same issue with say G1 if you were using
> -XX:-UseCompressedClassPointers, no?
>
> cheers,
> Per
>
> > 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 at oracle.com
> > <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at 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 at google.com
> >>     <mailto:jcbeyler at 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 at oracle.com
> >>         <mailto:daniel.daugherty at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180808/dc6bb03d/attachment-0001.html>