RFR (S): 8004172: Update jstat counter names to reflect metaspace changes

[Stefan Karlsson]

On 28 jan 2013, at 16:14, Erik Helin <erik.helin at oracle.com> wrote:

> Jon,
> 
> thanks for your review!
> 
> On 01/24/2013 07:57 PM, Jon Masamitsu wrote:
>> I looked at the hotspot changes and they look correct.  But I'm
>> not sure that "sun.gc" should be in the name of the counter.  Maybe
>> use SUN_RT instead of SUN_GC.
> 
> 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.
> 
> I've also added better error handling if a Java Out Of Memory exceptions occur is raised in PerfDataManager::create_variable.
> 
> Finally, I've moved some common code to the function create_ms_variable.
> 
> Webrev:
> - hotspot: http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.01/

Would you mind using two indentation levels here:
+MetaspaceCounters::MetaspaceCounters() :
+  _capacity(NULL),
+  _used(NULL),
+  _max_capacity(NULL) {

I think it would be good to also extract this:
  68     const char *counter_name = PerfDataManager::counter_name(ms, "minCapacity");
  69     PerfDataManager::create_constant(SUN_RT, counter_name, PerfData::U_Bytes,

into a create_ms_constant, just like you did with create_ms_variable.

You should probably use CHECK instead of THREAD.
+    _max_capacity = create_ms_variable(ms, "maxCapacity", max_capacity, THREAD);
+    _capacity = create_ms_variable(ms, "capacity", curr_capacity, THREAD);
+    _used = create_ms_variable(ms, "used", used, THREAD);

I think it would be enough to assert that the variables are not NULL.
 void MetaspaceCounters::update_capacity() {
   assert(UsePerfData, "Should not be called unless being used");
   size_t capacity_in_bytes = MetaspaceAux::capacity_in_bytes();
+  if (_capacity != NULL) {

thanks,
StefanK

> - jdk: http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.01/
> 
> What do you think?
> 
> Thanks,
> Erik
> 
>> Jon
>> 
>> On 1/24/2013 2:51 AM, Erik Helin wrote:
>>> Hi all,
>>> 
>>> here are the HotSpot changes for fixing JDK-8004172. This change uses
>>> the new namespace "sun.gc.metaspace" for the metaspace counters and
>>> also removes some code from metaspaceCounters.hpp/cpp that is not
>>> needed any longer.
>>> 
>>> Note that the tests will continue to fail until the JDK part of the
>>> change finds it way into the hotspot-gc forest.
>>> 
>>> The JDK part of the change is also out for review on
>>> serviceability-dev at openjdk.java.net.
>>> 
>>> Webrev:
>>> HotSpot: http://cr.openjdk.java.net/~ehelin/8004172/hotspot/webrev.00/
>>> JDK: http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.00/
>>> 
>>> Bug:
>>> http://bugs.sun.com/view_bug.do?bug_id=8004172
>>> 
>>> Testing:
>>> Run the jstat jtreg tests locally on my machine on a repository where
>>> I've applied both the JDK changes and the HotSpot changes.
>>> 
>>> Thanks,
>>> Erik
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130130/bade3711/attachment.htm>