Re: RFR(S) 8207765: HeapMonitorIntervalRateTest fails with ZGC

[Alex Menkov]

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