[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8175312: SA: clhsdb: Provide an improved heap summary for 'universe' for G1GC
From:       Jini George <jini.george () oracle ! com>
Date:       2018-03-15 4:33:13
Message-ID: b0ba0948-3d8d-b230-593c-f41e3eb1132a () oracle ! com
[Download RAW message or body]

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.george@oracle.com 
> <mailto:jini.george@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.
> 
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]