<div dir="ltr">Jini,<div><br></div><div> Looks good. One minor comment:</div><div><pre style="color:rgb(0,0,0);font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span class="gmail-new" style="color:blue;font-weight:normal">+ public void printG1HeapSummary(G1CollectedHeap heap) {</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+ G1CollectedHeap g1h = (G1CollectedHeap) heap;</span></pre> </div><div> 'heap' has been cast to '<span style="color:blue">G1CollectedHeap' at call site, so seems no need to convert here again.</span></div><div><span style="color:blue"><br></span></div><div><span style="color:blue">Thanks</span></div><div><span style="color:blue">Yumin</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 12, 2018 at 8:52 AM, Jini George <span dir="ltr"><<a href="mailto:jini.george@oracle.com" target="_blank">jini.george@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thank you very much, Stefan. Could one more reviewer please take a look at it ?<span class="HOEnZb"><font color="#888888"><br>
<br>
- Jini.</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 3/12/2018 8:52 PM, Stefan Johansson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Jini,<br>
<br>
This looks good. I'm totally fine with skipping metaspace if that isn't displayed for the other GCs.<br>
<br>
Cheers,<br>
Stefan<br>
<br>
On 2018-03-09 10:29, Jini George wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Here is the revised webrev:<br>
<br>
<a href="http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jg<wbr>eorge/8175312/webrev.02/</a><br>
<br>
I have made modifications to have the 'universe' command display details like:<br>
<br>
hsdb> universe<br>
Heap Parameters:<br>
garbage-first heap [0x0000000725200000, 0x00000007c0000000] region size 1024K<br>
G1 Heap:<br>
regions = 2478<br>
capacity = 2598371328 (2478.0MB)<br>
used = 5242880 (5.0MB)<br>
free = 2593128448 (2473.0MB)<br>
0.20177562550443906% used<br>
G1 Young Generation:<br>
Eden Space:<br>
regions = 5<br>
capacity = 8388608 (8.0MB)<br>
used = 5242880 (5.0MB)<br>
free = 3145728 (3.0MB)<br>
62.5% used<br>
Survivor Space:<br>
regions = 0<br>
capacity = 0 (0.0MB)<br>
used = 0 (0.0MB)<br>
free = 0 (0.0MB)<br>
0.0% used<br>
G1 Old Generation:<br>
regions = 0<br>
capacity = 155189248 (148.0MB)<br>
used = 0 (0.0MB)<br>
free = 155189248 (148.0MB)<br>
0.0% used<br>
<br>
<br>
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.<br>
<br>
hsdb> g1regiondetails<br>
Region Details:<br>
Region: 0x0000000725200000,0x000000072<wbr>5200000,0x0000000725300000:<wbr>Free<br>
Region: 0x0000000725300000,0x000000072<wbr>5300000,0x0000000725400000:<wbr>Free<br>
Region: 0x0000000725400000,0x000000072<wbr>5400000,0x0000000725500000:<wbr>Free<br>
Region: 0x0000000725500000,0x000000072<wbr>5500000,0x0000000725600000:<wbr>Free<br>
Region: 0x0000000725600000,0x000000072<wbr>5600000,0x0000000725700000:<wbr>Free<br>
Region: 0x0000000725700000,0x000000072<wbr>5700000,0x0000000725800000:<wbr>Free<br>
...<br>
<br>
Thanks,<br>
Jini.<br>
<br>
<br>
On 2/28/2018 12:56 PM, Jini George wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thank you very much, Stefan. My answers inline.<br>
<br>
On 2/27/2018 3:30 PM, Stefan Johansson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Jini,<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
JIRA ID:<a href="https://bugs.openjdk.java.net/browse/JDK-8175312" rel="noreferrer" target="_blank">https://bugs.openjdk.java.n<wbr>et/browse/JDK-8175312</a><br>
Webrev: <a href="http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jg<wbr>eorge/8175312/webrev.00/index.<wbr>html</a><br>
<br>
</blockquote></blockquote>
It looks like a file is missing, did you forget to add it to the changeset?<br>
</blockquote>
<br>
Indeed, I had missed that! I added the missing file in the following webrev:<br>
<br>
<a href="http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jg<wbr>eorge/8175312/webrev.01/</a><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
---<br>
open/src/jdk.hotspot.agent/sha<wbr>re/classes/sun/jvm/hotspot/gc/<wbr>g1/G1CollectedHeap.java:36: error: cannot find symbol<br>
import sun.jvm.hotspot.gc.shared.Prin<wbr>tRegionClosure;<br>
---<br>
<br>
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:<br>
garbage-first heap total 16384K, used 3072K [0x00000000ff000000, 0x0000000100000000)<br>
region size 1024K, 4 young (4096K), 0 survivors (0K)<br>
Metaspace used 6731K, capacity 6825K, committed 7040K, reserved 1056768K<br>
class space used 559K, capacity 594K, committed 640K, reserved 1048576K<br>
</blockquote>
<br>
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.<br>
<br>
Thanks,<br>
Jini.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Stefan<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Modifications have been made to display the regions like:<br>
<br>
...<br>
Region: 0x00000005c5400000,0x00000005c<wbr>5600000,0x00000005c5600000:Old<br>
Region: 0x00000005c5600000,0x00000005c<wbr>5800000,0x00000005c5800000:Old<br>
Region: 0x00000005c5800000,0x00000005c<wbr>5a00000,0x00000005c5a00000:Old<br>
Region: 0x00000005c5a00000,0x00000005c<wbr>5c00000,0x00000005c5c00000:Old<br>
Region: 0x00000005c5c00000,0x00000005c<wbr>5c00000,0x00000005c5e00000:<wbr>Free<br>
Region: 0x00000005c5e00000,0x00000005c<wbr>5e00000,0x00000005c6000000:<wbr>Free<br>
Region: 0x00000005c6000000,0x00000005c<wbr>6200000,0x00000005c6200000:Old<br>
...<br>
<br>
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.<br>
<br>
The SA tests pass with jprt and Mach5.<br>
<br>
Thanks,<br>
Jini.<br>
</blockquote></blockquote>
<br>
</blockquote></blockquote></blockquote>
<br>
</blockquote>
</div></div></blockquote></div><br></div>