Thank you very much, Yumin. The modified webrev (after incorporating your comment) is at:

http://cr.openjdk.java.net/~jgeorge/8175312/webrev.03/index.html

Thanks,
Jini.

On 3/13/2018 6:24 AM, yumin qi wrote:
Jini,

   Looks good. One minor comment:

+ public void printG1HeapSummary(G1CollectedHeap heap) {
+ G1CollectedHeap g1h = (G1CollectedHeap) heap;

 'heap' has been cast to 'G1CollectedHeap' at call site, so seems no need to convert here again.

Thanks
Yumin

On Mon, Mar 12, 2018 at 8:52 AM, Jini George <jini.geo...@oracle.com <mailto:jini.geo...@oracle.com>> wrote:

    Thank you very much, Stefan. Could one more reviewer please take a
    look at it ?

    - Jini.


    On 3/12/2018 8:52 PM, Stefan Johansson wrote:

        Hi Jini,

        This looks good. I'm totally fine with skipping metaspace if
        that isn't displayed for the other GCs.

        Cheers,
        Stefan

        On 2018-03-09 10:29, Jini George wrote:

            Here is the revised webrev:

            http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/
            <http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/>

            I have made modifications to have the 'universe' command
            display details like:

            hsdb> universe
            Heap Parameters:
            garbage-first heap [0x0000000725200000, 0x00000007c0000000]
            region size 1024K
            G1 Heap:
                regions  = 2478
                capacity = 2598371328 (2478.0MB)
                used     = 5242880 (5.0MB)
                free     = 2593128448 (2473.0MB)
                0.20177562550443906% used
            G1 Young Generation:
            Eden Space:
                regions  = 5
                capacity = 8388608 (8.0MB)
                used     = 5242880 (5.0MB)
                free     = 3145728 (3.0MB)
                62.5% used
            Survivor Space:
                regions  = 0
                capacity = 0 (0.0MB)
                used     = 0 (0.0MB)
                free     = 0 (0.0MB)
                0.0% used
            G1 Old Generation:
                regions  = 0
                capacity = 155189248 (148.0MB)
                used     = 0 (0.0MB)
                free     = 155189248 (148.0MB)
                0.0% used


            I did not add the metaspace details since that did not seem
            to be in line with the 'universe' output for other GCs. I
            have added a new command "g1regiondetails" to display the
            region details, and have modified the tests accordingly.

            hsdb> g1regiondetails
            Region Details:
            Region:
            0x0000000725200000,0x0000000725200000,0x0000000725300000:Free
            Region:
            0x0000000725300000,0x0000000725300000,0x0000000725400000:Free
            Region:
            0x0000000725400000,0x0000000725400000,0x0000000725500000:Free
            Region:
            0x0000000725500000,0x0000000725500000,0x0000000725600000:Free
            Region:
            0x0000000725600000,0x0000000725600000,0x0000000725700000:Free
            Region:
            0x0000000725700000,0x0000000725700000,0x0000000725800000:Free
            ...

            Thanks,
            Jini.


            On 2/28/2018 12:56 PM, Jini George wrote:

                Thank you very much, Stefan. My answers inline.

                On 2/27/2018 3:30 PM, Stefan Johansson wrote:

                    Hi Jini,


                            JIRA
                            ID:https://bugs.openjdk.java.net/browse/JDK-8175312
                            <https://bugs.openjdk.java.net/browse/JDK-8175312>
                            Webrev:
                            
http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.html
                            
<http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.html>

                    It looks like a file is missing, did you forget to
                    add it to the changeset?


                Indeed, I had missed that! I added the missing file in
                the following webrev:

                http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/
                <http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/>

                    ---
                    
open/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1CollectedHeap.java:36:
                    error: cannot find symbol
                    import sun.jvm.hotspot.gc.shared.PrintRegionClosure;
                    ---

                    Otherwise the change looks good, but I would like to
                    see the output live. For a big heap this will print
                    a lot of data, just wondering if the universe
                    command is the correct choice for this kind of
                    output. I like having the possibility to print all
                    regions, so I want the change but maybe it should be
                    a different command and 'universe' just prints a
                    little more than before. Something like our logging
                    heap-summary at shutdown:
                    garbage-first heap   total 16384K, used 3072K
                    [0x00000000ff000000, 0x0000000100000000)
                       region size 1024K, 4 young (4096K), 0 survivors (0K)
                    Metaspace       used 6731K, capacity 6825K,
                    committed 7040K, reserved 1056768K
                       class space    used 559K, capacity 594K,
                    committed 640K, reserved 1048576K


                Ok, will add this, and could probably have the region
                details displayed under a new command called
                "g1regiondetails", or some such, and send out a new webrev.

                Thanks,
                Jini.


                    Thanks,
                    Stefan

                            Modifications have been made to display the
                            regions like:

                            ...
                            Region:
                            
0x00000005c5400000,0x00000005c5600000,0x00000005c5600000:Old
                            Region:
                            
0x00000005c5600000,0x00000005c5800000,0x00000005c5800000:Old
                            Region:
                            
0x00000005c5800000,0x00000005c5a00000,0x00000005c5a00000:Old
                            Region:
                            
0x00000005c5a00000,0x00000005c5c00000,0x00000005c5c00000:Old
                            Region:
                            
0x00000005c5c00000,0x00000005c5c00000,0x00000005c5e00000:Free
                            Region:
                            
0x00000005c5e00000,0x00000005c5e00000,0x00000005c6000000:Free
                            Region:
                            
0x00000005c6000000,0x00000005c6200000,0x00000005c6200000:Old
                            ...

                            The jtreg test at this point does not
                            include any testing for the display of
                            archived or pinned regions. The testing for
                            this will be added once JDK-8174994 is resolved.

                            The SA tests pass with jprt and Mach5.

                            Thanks,
                            Jini.




Reply via email to