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" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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
