<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><div><br></div><div>On 28 jan 2013, at 16:14, Erik Helin <<a href="mailto:erik.helin@oracle.com">erik.helin@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Jon,<br><br>thanks for your review!<br><br>On 01/24/2013 07:57 PM, Jon Masamitsu wrote:<br><blockquote type="cite">I looked at the hotspot changes and they look correct. But I'm<br>not sure that "sun.gc" should be in the name of the counter. Maybe<br>use SUN_RT instead of SUN_GC.<br></blockquote><br>I've updated the code to use the SUN_RT namespace instead of the SUN_GC namespace. This also required changes to the JDK code.<br><br>I've also added better error handling if a Java Out Of Memory exceptions occur is raised in PerfDataManager::create_variable.<br><br>Finally, I've moved some common code to the function create_ms_variable.<br><br>Webrev:<br>- hotspot: <a href="http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.01/">http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.01/</a><br></blockquote><div><br></div><div><div style="margin: 0px; font-size: 12px; font-family: Helvetica; "><div style="margin: 0px; ">Would you mind using two indentation levels here:</div><div style="margin: 0px; font-family: Courier; ">+MetaspaceCounters::MetaspaceCounters() :</div><div style="margin: 0px; font-family: Courier; ">+ _capacity(NULL),</div><div style="margin: 0px; font-family: Courier; ">+ _used(NULL),</div><div style="margin: 0px; font-family: Courier; ">+ _max_capacity(NULL) {</div><div style="margin: 0px; min-height: 14px; "><br></div><div style="margin: 0px; ">I think it would be good to also extract this:</div><div style="margin: 0px; font-family: Courier; "> 68 const char *counter_name = PerfDataManager::counter_name(ms, "minCapacity");</div><div style="margin: 0px; font-family: Courier; "> 69 PerfDataManager::create_constant(SUN_RT, counter_name, PerfData::U_Bytes,</div><div style="margin: 0px; min-height: 14px; "><br></div><div style="margin: 0px; ">into a create_ms_constant, just like you did with create_ms_variable.</div><div style="margin: 0px; min-height: 14px; "><br></div><div style="margin: 0px; ">You should probably use CHECK instead of THREAD.</div><div style="margin: 0px; font-size: 13px; font-family: Courier; ">+ _max_capacity = create_ms_variable(ms, "maxCapacity", max_capacity, THREAD);</div><div style="margin: 0px; font-size: 13px; font-family: Courier; ">+ _capacity = create_ms_variable(ms, "capacity", curr_capacity, THREAD);</div><div style="margin: 0px; font-size: 13px; font-family: Courier; ">+ _used = create_ms_variable(ms, "used", used, THREAD);</div><div style="margin: 0px; font-size: 13px; font-family: Courier; min-height: 16px; "><br></div><div style="margin: 0px; font-size: 13px; font-family: Courier; ">I think it would be enough to assert that the variables are not NULL.</div><div style="margin: 0px; font-family: Courier; "> void MetaspaceCounters::update_capacity() {</div><div style="margin: 0px; font-family: Courier; "> assert(UsePerfData, "Should not be called unless being used");</div><div style="margin: 0px; font-family: Courier; "> size_t capacity_in_bytes = MetaspaceAux::capacity_in_bytes();</div><div style="margin: 0px; font-family: Courier; ">+ if (_capacity != NULL) {</div><div style="margin: 0px; font-family: Courier; min-height: 14px; "><br></div><div style="margin: 0px; font-family: Courier; ">thanks,</div><div style="margin: 0px; font-family: Courier; ">StefanK</div></div><div style="margin: 0px; font-size: 12px; font-family: Helvetica; "><br></div></div><blockquote type="cite">- jdk: <a href="http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.01/">http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.01/</a><br><br>What do you think?<br><br>Thanks,<br>Erik<br><br><blockquote type="cite">Jon<br><br>On 1/24/2013 2:51 AM, Erik Helin wrote:<br><blockquote type="cite">Hi all,<br><br>here are the HotSpot changes for fixing JDK-8004172. This change uses<br>the new namespace "sun.gc.metaspace" for the metaspace counters and<br>also removes some code from metaspaceCounters.hpp/cpp that is not<br>needed any longer.<br><br>Note that the tests will continue to fail until the JDK part of the<br>change finds it way into the hotspot-gc forest.<br><br>The JDK part of the change is also out for review on<br><a href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>.<br><br>Webrev:<br>HotSpot: <a href="http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.00/">http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.00/</a><br>JDK: <a href="http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.00/">http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.00/</a><br><br>Bug:<br><a href="http://bugs.sun.com/view_bug.do?bug_id=8004172">http://bugs.sun.com/view_bug.do?bug_id=8004172</a><br><br>Testing:<br>Run the jstat jtreg tests locally on my machine on a repository where<br>I've applied both the JDK changes and the HotSpot changes.<br><br>Thanks,<br>Erik<br></blockquote></blockquote><br></blockquote></div><br></body></html>