RFR(L): 8196989: Revamp G1 JMX MemoryPoolMXBean, GarbageCollectorMXBean, and jstat counter definitions

[Thomas Schatzl]

Hi Paul,

  Erik may not have time in the next few months to review such a large
change. But it would also be better to do the changes in steps for
other reviewers. Also see below.

On Mon, 2018-07-23 at 21:33 +0000, Hohensee, Paul wrote: 
> Please review.
>  
> Bug: https://bugs.openjdk.java.net/browse/JDK-8196989

I may have missed this in the previous discussion (which has been a
while), but has there been any discussion about a "Free (Region) Space"
for the committed but free regions?

It seems a bit random to assign free region to the "old space",
seemingly just a repeat of the current behavior (where everything has
been put into "old gen").

Also, imho the second survivor space should preferably be dropped
completely. :)

> CSR: https://bugs.openjdk.java.net/browse/JDK-8196991
> Webrev: http://cr.openjdk.java.net/~phh/8196989/webrev.00
>  
> This webrev is marked ‘L’ because it’s a behavioral change (CSR in
> draft state, may I have a review of that too please?) and because the
> test change fanout is large. The actual code changes are ‘M’.
>  
> Passes the submit repo, Hotspot tier1, the JFR gc event tests and any
> other test set with ‘gc’ or ‘serviceability’ in the test directory
> name. I found it difficult to verify the accuracy of the reported
> values other than manually, since they can vary from run to run of
> the same program. I’d appreciate suggestions for how to go about
> writing accuracy tests.
>  
> I set out originally to revamp only the MXBeans, but decided it would
> be incomplete if I didn’t include the jstat counters and the output
> of the GC.heap_info jcmd option. I can separate the latter two into
> their own RFEs, but I find it easier understand it all in a single
> webrev and hope the reviewers will too.
>  
> The basic approach is to add the new memory pools and collectors, the
> new jstat counters, and an archive region counter that stands in for
> an actual archive region set. HeapRegionSets are disjoint, so 

One option would be to add a HeapRegionSet tailored for archives that
does not check the disjoint-criteria (it is superficially used for
verification only anyway) - we already have special classes/flags for
different kinds of regions (humongous/free/old) in the HeapRegionSet
hierarchy.

> initially I tried to create a first-class archive region set (on the
> same level as the humongous region set), but that idea foundered on
> the fact that there’s too much code I don’t fully understand that
> depends on archive regions being in the existing old region set. 

Probably to simplify the implementation of archive regions :)

This is another option, and does not look too bad actually, we only
need to check and change all HeapRegion::is_old() or
HeapRegion::is_old_or_humongous() checks.

Now we only need a good name for is_old_or_archive_or_humongous()
because that one is a bit lengthy :)

> Externally (i.e., in the MXBeans and the jstat counters), however,
> the old region set doesn’t include archive regions (unless running in
> legacy mode).
>  
> I used CMS’s TraceCMSMemoryManagerStats class as the model for
> TraceConcMemoryManagerStats, which latter collects statistics on
> concurrent cycles. There are two STW pauses in each concurrent cycle:
> they are recorded separately and count as two sun.gc.collector.2
> events.

I would like to move away serviceability code from
G1CollectedHeap.h/cpp as much as possible; e.g. it would be very nice
to make G1MonitoringSupport the owner of all the serviceability related
data. Also the _use_legacy_monitoring member should probably move there
too.

> The humongous and archive space committed and used values are always
> identical,

This is because, for some reason, G1 counts the memory filled with
filler objects as "used". Other collectors don't.

> hence they are always 100% used.

You may have noticed that just recently we got a bug (https://bugs.open
jdk.java.net/browse/JDK-8207200) filed against the G1 MXBeans because
of races in the code particularly code to be not-racy.

The reason is the really weird calculation of used/committed for eden
space/survivor space/old gen and that the precondition written down in
G1MonitoringSupport::recalculate_sizes() does not hold.

G1 MemoryMXBeans basically fabricates some numbers as you might have
noticed :), so in addition to fixing that issue with the race I am
still working on improving the accuracy of the used values.
Also, in course of this change I am considering removing some other
backwards-bending in returned values for G1 (the mentioned and e.g.
funky stuff like assuming that adding together max-capacities of the
pools gives you total heap size).

I have also a preliminary webrev for that at http://cr.openjdk.java.net
/~tschatzl/8207200/webrev/ which unfortunately clashes a lot with your
changes. The reason why it is a single webrev is because I am not
finished yet - I tend to split it up in parts for much better reviewing
at the very end only.

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.
- move serviceability stuff as much as possible to g1MonitoringSupport
- 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.

> The revised output of jcmd GC.heap_info is in
> G1CollectedHeap::print_on().
> I fixed a typo in src/hotspot/share/gc/g1/g1Policy.hpp by changing
> the result type of young_list_target_length() from size_t to uint,
> which latter is the type of the _young_list_target_length member.
> I updated the copyright date in
> src/hotspot/share/services/memoryService.hpp to 2018, as I neglected
> to do so in a previous push.

Comments?

Thanks,
  Thomas