<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>I looked at this but it took time for me to "digest" it and I haven't entirely gone through the real GC changes :)</div><div><br></div><div>My few remarks on the webrev itself are:</div><div> - <a href="http://cr.openjdk.java.net/~phh/8196989/webrev.02/src/hotspot/share/services/memoryService.hpp.udiff.html">http://cr.openjdk.java.net/~phh/8196989/webrev.02/src/hotspot/share/services/memoryService.hpp.udiff.html</a></div><div> - There is no need to change the copyright, right?</div><div><br></div><div> - <a href="http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java.udiff.html">http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java.udiff.html</a></div><div> - the break seems to be on the wrong line, no?</div><div> </div><div>+ } break;<br></div><div> </div><div><br></div><div> - <a href="http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html">http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html</a><br></div><div> and</div><div> <a href="http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/MemoryManagement.java.udiff.html">http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/MemoryManagement.java.udiff.html</a><br></div><div><br></div><div><div>+ // In G1, humongous objects are tracked in the old space only in<br></div><div>+ // legacy monitoring mode. In default mode, G1 tracks humongous</div></div><div><div>+ // objects in the humongous space, which latter also supports a</div><div>+ // usage threshold. Since we're allocating humongous objects in</div><div>+ // this test, in default mode the old space doesn't change. For</div><div>+ // this test, we use the old space in legacy mode (it's called</div><div>+ // "G1 Old Gen" and the humongous space in default mode. If we</div><div>+ // used "G1 Old Space" in default mode, notification would never</div><div>+ // happen.</div></div><div><br></div><div>-> latter seems ot be the wrong word or something is missing in that sentence</div><div>-> the parenthesis is never closed (it's called.... is missing a ) somewhere</div><div><br></div><div>Thanks,</div><div>Jc</div></div></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 17, 2018 at 1:18 PM Hohensee, Paul <<a href="mailto:hohensee@amazon.com">hohensee@amazon.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="#0563C1" vlink="#954F72">
<div class="m_-5496290937553094592WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt">Ping.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt"><u></u> <u></u></span></p>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:black">From: </span></b><span style="color:black">serviceability-dev <<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">serviceability-dev-bounces@openjdk.java.net</a>> on behalf of "Hohensee, Paul" <<a href="mailto:hohensee@amazon.com" target="_blank">hohensee@amazon.com</a>><br>
<b>Date: </b>Thursday, October 11, 2018 at 6:46 PM<br>
<b>To: </b>"<a href="mailto:hotspot-gc-dev@openjdk.java.net" target="_blank">hotspot-gc-dev@openjdk.java.net</a>" <<a href="mailto:hotspot-gc-dev@openjdk.java.net" target="_blank">hotspot-gc-dev@openjdk.java.net</a>>, "<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>" <<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>><br>
<b>Subject: </b>Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt"><u></u> <u></u></span></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt">Any takers? :)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:black">From: </span></b><span style="color:black">serviceability-dev <<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">serviceability-dev-bounces@openjdk.java.net</a>> on behalf of "Hohensee, Paul" <<a href="mailto:hohensee@amazon.com" target="_blank">hohensee@amazon.com</a>><br>
<b>Date: </b>Monday, October 8, 2018 at 7:50 PM<br>
<b>To: </b>"<a href="mailto:hotspot-gc-dev@openjdk.java.net" target="_blank">hotspot-gc-dev@openjdk.java.net</a>" <<a href="mailto:hotspot-gc-dev@openjdk.java.net" target="_blank">hotspot-gc-dev@openjdk.java.net</a>>, "<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>" <<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>><br>
<b>Subject: </b>RFR: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt">Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8196989" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8196989</a></span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">CSR: <a href="https://bugs.openjdk.java.net/browse/JDK-8196991" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8196991</a></span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">Webrev: <a href="http://cr.openjdk.java.net/~phh/8196989/webrev.02/" target="_blank">http://cr.openjdk.java.net/~phh/8196989/webrev.02/</a></span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">As requested, I split the jstat counter update off from the MXBean update. This is the MXBean update. The jstat counter RFE is
<a href="https://bugs.openjdk.java.net/browse/JDK-8210965" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210965</a> and its CSR is
<a href="https://bugs.openjdk.java.net/browse/JDK-8210966" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210966</a>.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">The MXBean CSR is in draft state, I’d greatly appreciate review and sign-off.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">It’s been suggested that we add another pool to represent the free region set, but doing so would be incompatible with existing MXBean use invariants for all GCs. These are:</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The sum of the pools’ MemoryUsage.max properties is the total reserved heap size.</span><u></u><u></u></li><li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The sum of the pools’ MemoryUsage.committed properties is the total committed size.</span><u></u><u></u></li><li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The sum of the pools’ MemoryUsage.used properties is the total size of the memory containing objects, live and dead-and-yet-to-be-collected, as the case
might be, plus intentional gaps between them.</span><u></u><u></u></li><li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The total free space is (sum of the max properties – sum of the used properties).</span><u></u><u></u></li><li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The total uncommitted space is (sum of the max properties – sum of the committed properties).</span><u></u><u></u></li><li class="m_-5496290937553094592MsoListParagraph" style="margin-left:0in"><span style="font-size:11.0pt">The total committed free space is (2) – (3).</span><u></u><u></u></li></ol>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">To keep invariants 1, 2 and 3, the free region pool’s “max” property should be “undefined” (i.e., -1). The intuitive, to me, “used” property value would be the total free space, but that would violate invariant
4 above. Defining the “committed” property as the total committed free space would violate invariants 2 and 6.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">The patch passes the submit repo, hotspot tier1, and, separately, the serviceability, jfr, and gc jtreg tests. I’m uncertain how to construct a test that checks for valid MXBean content: the existing tests
don’t. Any such test will be fragile due to possible future Hotspot changes that affect the values, and to run-to-run variability. I’ve done by-hand comparisons between the old and new MXBean content using the SwingSet2 demo, including using App CDS, and the
numbers look reasonable.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">The guts of the change are in G1MonitoringSupport::recalculate_sizes, initialize_serviceability, memory_managers, memory_pools, and G1MonitoringScope. I also defined TraceConcMemoryManagerStats to track the
concurrent cycle in a way analogous to TraceCMSMemoryManagerStats. The changes to the includes in g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp are to satisfy compiler complaints. I changed the 3<sup>rd</sup> argument to the G1MonitoringScope constructor
to a mixed_gc flag, and use accessor methods instead of direct field accesses when accessor methods exist. I believe I’ve minimized the latter. I updated the copyright date to 2018 in memoryService.hpp because I neglected to do so in my previous G1 MXBean
patch.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">Thanks,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt">Paul</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><u></u><u></u></p>
</div>
</div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>