RFR: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

[Thomas Schatzl]

Hi Paul,

  sorry for the long wait.

On Mon, 2018-10-08 at 23:49 +0000, Hohensee, Paul wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8196989
> CSR: https://bugs.openjdk.java.net/browse/JDK-8196991
> Webrev: http://cr.openjdk.java.net/~phh/8196989/webrev.02/

Note that I reviewed the latest change at 

http://cr.openjdk.java.net/~phh/8196989/webrev.04/

Currently pushing this through our testing.

At that point the thread itself has moved to a question of a particular
detail, with too many indentations to the email text I wanted to
actually answer, so I answered to the original email. :)

>  
> As requested, I split the jstat counter update off from the MXBean
> update. This is the MXBean update. The jstat counter RFE is 
> https://bugs.openjdk.java.net/browse/JDK-8210965 and its CSR is 
> https://bugs.openjdk.java.net/browse/JDK-8210966.
>  
> The MXBean CSR is in draft state, I’d greatly appreciate review and
> sign-off.

I will look at it. As David already mentioned, it also needs to mention
the new flag regardless of whether introducing it as deprecated or not.

> It’s been suggested that we add another pool to represent the free
> region set, but doing so would be incompatible with existing MXBean
> use invariants for all GCs. These are:
>  
> 1) The sum of the pools’ MemoryUsage.max properties is the total
> reserved heap size.
> 2) The sum of the pools’ MemoryUsage.committed properties is the
> total committed size.
> 3) The sum of the pools’ MemoryUsage.used properties is the total
> size of the memory containing objects, live and dead-and-yet-to-be-
> collected, as the case might be, plus intentional gaps between them.
> 4) The total free space is (sum of the max properties – sum of the
> used properties).
> 5) The total uncommitted space is (sum of the max properties – sum of
> the committed properties).
> 6) The total committed free space is (2) – (3).
>  

I think we discussed this before, but none of these so-called
invariants are specified anywhere and artifacts of the particular heap
layout and MXBean implementations for Serial GC/Parallel GC and CMS.

My understanding is that there are implementations out there that
assume just that - but those can just use the "legacy" mxbeans.

Currently, the MXBean values for G1 have been hacked to conform to this
scheme for "compatibility reasons" (e.g. eden regions max is -1; or
just look at the sorry code bending over backwards in
recalculate_sizes()). I would prefer to not carry on with this,
particularly with the new collectors (ZGC and Shenandoah) in the future
in mind, with a potential "young gen" going to have the same issues
(not sure about Shenandoah going to have something like this, but I
assume so for various reasons).

This change would be a good opportunity to do things correctly and
return values as the VM sees them (like proper max values for all
MXBeans), and avoid another round of having a "UseLegacyMemoryMXBeans2"
switch later.

Additionally most of these invariants do not even hold for the "old"
collectors if you look closely. One simple case is a GC occuring
between readings of the MemoryMXBean (with the known workaround retry
until you get consisten values instead of calling the right method),
and the issue related to JDK-8207200 (which *exactly* did rely on these
"invariants", and was broken because of that).

I also want to point out that the jstat counters already return heap
size as max capacity (which probably most of the MemoryMXBeans would
also return if fixed).

So overall I honestly do not like the scope of this change, i.e. this
half-fixing of G1 MemoryMXBeans for all these reasons (while the change
itself is good).

Changing the values returned by MemoryMXBeans would not decrease the
amount of information or quality you get, but rather improve both, and
make sure that future generational enhancements of collectors won't
have to bend backwards for broken code that assumes non-"invariants"
again.

> To keep invariants 1, 2 and 3, the free region pool’s “max” property
> should be “undefined” (i.e., -1). The intuitive, to me, “used”
> property value would be the total free space, but that would violate
> invariant 4 above. Defining the “committed” property as the total
> committed free space would violate invariants 2 and 6.

My intuitive take on the values of the Free region space would be:
  - used = 0, there are never objects in there, it's the *free* space
after all.
  - committed = number of regions committed to Free regions
  - max = current max heap size - other committed size, as this would
then also encompass the so-called "Not Available" Free regions in G1.
It would probably be too complicated and not relevant to the users to
have an extra MXBean for those.

> The patch passes the submit repo, hotspot tier1, and, separately, the
> serviceability, jfr, and gc jtreg tests. I’m uncertain how to
> construct a test that checks for valid MXBean content: the existing
> tests don’t. Any such test will be fragile due to possible future
> Hotspot changes that affect the values, and to run-to-run
> variability. I’ve done by-hand comparisons between the old and new
> MXBean content using the SwingSet2 demo, including using App CDS, and
> the numbers look reasonable.
>  
> The guts of the change are in G1MonitoringSupport::recalculate_sizes,
> initialize_serviceability, memory_managers, memory_pools, and
> G1MonitoringScope. I also defined TraceConcMemoryManagerStats to
> track the concurrent cycle in a way analogous to
> TraceCMSMemoryManagerStats. The changes to the includes in
> g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp are to satisfy
> compiler complaints. I changed the 3rd argument to the
> G1MonitoringScope constructor to a mixed_gc flag, and use accessor
> methods instead of direct field accesses when accessor methods exist.
> I believe I’ve minimized the latter. I updated the copyright date to
> 2018 in memoryService.hpp because I neglected to do so in my previous
> G1 MXBean patch.

  - I am not sure about the reasons for the change in
G1CollectedHeap::post_initialize(). Did you experience some problems,
or is this some cleanup?

  - the comment in g1Collected.hpp:1224 is misplaced, if you touch it,
please remove. The "surv rate groups" moved to G1Policy long ago.

  - the two "Record start/end, but take no time" comments in
g1ConcurrentMark.cpp seem to not be very useful. The call below tells
you that exactly that happens, but not why, adding no information. I
would remove them.

  - the comments at the top of g1MemoryPool.hpp and
g1MemoryPool.hpp:66-68 and 95-98 are nice but seem to mostly duplicate
the information in the comment in g1MonitoringSupport.hpp. Maybe they
could be merged a bit to avoid having too much duplicate information
around (and refer to it in g1MemoryPool).

  - in the G1MonitoringSupport constructor, please avoid specifying
multiple member variable initializations in the same line using
additional formatting via spaces (I'm fine with additional newlines if
you want to group them). That is somewhat uncommon to read.

  - g1MonitoringSupport.cpp:288: please remove the unused variable
old_regions_count.

  - the comment in g1MonitoringSupport.hpp:277 needs to be made more
concise (thanks for fixing up lots of comments in this change!) by
adding something like:

"Values between different returned MemoryUsage may not be consistent
with each other." 

  - memoryManager.cpp:214/248: please add braces around the return
statement.

Rest of the changes seems good.

Thanks,
  Thomas