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.li...@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.spit...@oracle.com > > <mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com > > <mailto:serguei.spit...@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 <jcbey...@google.com > >> <mailto:jcbey...@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.daughe...@oracle.com > >> <mailto:daniel.daughe...@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
