8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

[JC Beyler]

Hi Paul,

I looked at this but it took time for me to "digest" it and I haven't
entirely gone through the real GC changes :)

My few remarks on the webrev itself are:
   -
http://cr.openjdk.java.net/~phh/8196989/webrev.02/src/hotspot/share/services/memoryService.hpp.udiff.html
      - There is no need to change the copyright, right?

  -
http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java.udiff.html
     - the break seems to be on the wrong line, no?

+                }                break;


    -
http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html
    and

http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/MemoryManagement.java.udiff.html

+                        // In G1, humongous objects are tracked in the old
space only in
+                        // legacy monitoring mode. In default mode, G1
tracks humongous
+                        // objects in the humongous space, which latter
also supports a
+                        // usage threshold. Since we're allocating
humongous objects in
+                        // this test, in default mode the old space
doesn't change. For
+                        // this test, we use the old space in legacy mode
(it's called
+                        // "G1 Old Gen" and the humongous space in default
mode. If we
+                        // used "G1 Old Space" in default mode,
notification would never
+                        // happen.

-> latter seems ot be the wrong word or something is missing in that
sentence
-> the parenthesis is never closed (it's called.... is missing a ) somewhere

Thanks,
Jc

On Wed, Oct 17, 2018 at 1:18 PM Hohensee, Paul <hohensee at amazon.com> wrote:

> Ping.
>
>
>
> *From: *serviceability-dev <serviceability-dev-bounces at openjdk.java.net>
> on behalf of "Hohensee, Paul" <hohensee at amazon.com>
> *Date: *Thursday, October 11, 2018 at 6:46 PM
> *To: *"hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>,
> "serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net
> >
> *Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector
> MXBean definitions
>
>
>
> Any takers? :)
>
>
>
> *From: *serviceability-dev <serviceability-dev-bounces at openjdk.java.net>
> on behalf of "Hohensee, Paul" <hohensee at amazon.com>
> *Date: *Monday, October 8, 2018 at 7:50 PM
> *To: *"hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>,
> "serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net
> >
> *Subject: *RFR: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector
> MXBean definitions
>
>
>
> 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/
>
>
>
> 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.
>
>
>
> 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).
>
>
>
> 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.
>
>
>
> 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.
>
>
>
> Thanks,
>
>
>
> Paul
>
>
>
>
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181017/ac7d33bf/attachment-0001.html>