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

List:       openjdk-serviceability-dev
Subject:    Re: RFR(L): 8196989: Revamp G1 JMX MemoryPoolMXBean, GarbageCollectorMXBean, and jstat counter defin
From:       "Hohensee, Paul" <hohensee () amazon ! com>
Date:       2018-07-31 18:45:14
Message-ID: C524108B-A7A9-49F7-9C3E-D1C3B74512CA () amazon ! com
[Download RAW message or body]

A few small things for http://cr.openjdk.java.net/~tschatzl/8208498/webrev/, \
otherwise looks good.

collectionSetChooser.cpp:
	Doesn't !r->is_old() include is_archive()?

g1CollectedHeap.hpp:
	Add archive_region_add(), archive_region_remove(), and old_set_bulk_remove().
	In non_young_capacity_bytes(), use old_regions_count(), humongous_regions_count(), \
and archive_regions_count().

g1CollectedHeap.cpp:
	Use old_set_add() and friends where possible.
	"// humongous regions set." -> "// humongous and archive region sets."

On 7/30/18, 4:27 PM, "Hohensee, Paul" <hohensee@amazon.com> wrote:

    A couple nits on \
http://cr.openjdk.java.net/~tschatzl/move-serviceability-stuff/webrev/.  
    g1CollectedHeap.cpp: in initialize_serviceability(), memory_managers(), and \
memory_pools(), use g1mm() instead of _g1mm.  
    g1MonitoringSupport.cpp: there's an extra newline after ~G1MonitoringSupport().
    
    Otherwise looks good.
    
    Paul
    
    On 7/30/18, 12:18 PM, "Hohensee, Paul" <hohensee@amazon.com> wrote:
    
        At JVMLS, so can't look in depth this instant, but I'm fine with your \
approach, except I'd get the new JMX and jstat structure in place before fixing the \
data that gets reported. Imo it'll be easier to fit correct data into the new \
JMX/jstat setup than into the old one, and doing it the new way will give us a good \
idea of exactly what we should do for the legacy ones.  
        Your archive region set webrev looks pretty much the same as what I wrote, \
but I got a trace trap when I tried to execute the resulting JVM. Not a clue why, so \
I abandoned it.  
        I'd not have thought of making a G1MonitoringScope, looks good.
        
        Thanks,
        
        Paul
        
        On 7/30/18, 6:04 AM, "Thomas Schatzl" <thomas.schatzl@oracle.com> wrote:
        
            Hi Paul,
            
              did some prototyping and wanted to show you the results and get your
            input:
            
            On Thu, 2018-07-26 at 16:06 +0200, Thomas Schatzl wrote:
            > 
            [...]
            > Could we work together on first refactoring the code before adding
            > new
            > kinds of spaces to the MXBeans?
            > 
            > Looking at this change and mine roughly the following issues would
            > need to be resolved first:
            > - find a solution for archive regions as suggested above :) At the
            > moment, without doing the change, I would tend to make archive
            > regions separate from old regions.
            
            I went with that and I am currently testing https://bugs.openjdk.java.n
            et/browse/JDK-8208498 ; here's a webrev to look at: http://cr.openjdk.j
            ava.net/~tschatzl/8208498/webrev/
            
            > - move serviceability stuff as much as possible to
            > g1MonitoringSupport
            
            Preliminary webrev:
            http://cr.openjdk.java.net/~tschatzl/move-serviceability-stuff/webrev/
            
            I think this came out better than expected: while we maybe want to add
            a ServiceabilitySupport interface that collects the
            get_memory_manager/pools/* methods in the future, imho this is a lot
            better than current code as it tightens the G1MonitoringSupport
            interface quite a bit.
            
            Particularly of note should be the G1MonitoringScope class that
            collects both TraceCollectorStats and TraceMemoryManagerStats into a
            single class. (Instead of the two bools passed to it something
            indicating the GC directly would probably be better too).
            
            It would be nice if something similar could be made for the concurrent
            Trace*Stats.
            
            > - clean up MemoryPool, remove duplicate information
            > - provide and return sane memory pool used/committed values to the
            > MXBeans
            > - clean up G1MonitoringSupport, e.g. avoid "*used/*committed"
            > variables
            > for every single memory pool. Use MemoryUsage structs for them. Make
            > reading of memory pool information atomic wrt to its readers (note
            > that I think it is currently just impossible to get consistent output
            > for other statistics like jstat) - that's JDK-8207200.
            > - add whatever serviceability stuff for the new pools/jstat/* in
            > steps.
            
            
            Thanks,
              Thomas
            
            
        
        
    
    


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