8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

Man Cao manc at google.com
Tue Oct 23 21:37:20 UTC 2018


Hi Paul,

I looked at the code in detail, and didn't find any major problem. A
few small issues below. I'm not a Reviewer, though.

----------------------------File Separator----------------------------------
In http://cr.openjdk.java.net/~phh/8196989/webrev.04/src/hotspot/share/gc/g1/g1MonitoringSupport.hpp.udiff.html

-//    - young_gen_used = current young region capacity
+//    - young_gen_committed = current young region capacity
 //    - survivor_used = survivor_capacity
 //    - eden_used = young_gen_used - survivor_used

"young_gen_used" is still referenced several times in comment,
although there is no function or field in code to compute
young_gen_used.
Maybe the following is more accurate?
// * Occupancy
//
//    - young_gen_committed = current young region capacity
//    - survivor_used = survivor_capacity
//    - eden_used = sum of eden regions allocated
//  In legacy mode:
//    - old_used = overall_used - survivor_used - eden_used
//  Otherwise:
//    - humongous_used = sum of humongous regions allocated
//    - archive_used = sum of archive regions allocated
//    - old_used = overall_used - survivor_used - eden_used -
//                         humongous_used - archive_used

----------------------------File Separator----------------------------------
In http://cr.openjdk.java.net/~phh/8196989/webrev.04/src/hotspot/share/gc/g1/g1CollectedHeap.cpp.udiff.html

 void G1CollectedHeap::post_initialize() {
+  // Necessary to satisfy locking discipline assertions.
+  MutexLockerEx x(Heap_lock);
+
   CollectedHeap::post_initialize();
   ref_processing_init();
 }

I couldn't immediately see which assertion requires the Heap_lock. Is
the lock required by the call to G1MonitoringSupport::update_sizes()
in G1MonitoringSupport::initialize_serviceability()?
Other collectors (Parallel, CMS, etc.) do not seem to hold the
Heap_lock for post_initialize() and initialize_serviceability(),
either.
Should this locking statement be put at a more general place, e.g.
universe_post_init() in universe.cpp; or if it is G1-specific, at a
place closer to where it is required in G1?

----------------------------File Separator----------------------------------
In http://cr.openjdk.java.net/~phh/8196989/webrev.04/src/hotspot/share/services/memoryManager.cpp.udiff.html

 void GCMemoryManager::gc_begin(bool recordGCBeginTime, bool recordPreGCUsage,
                                bool recordAccumulatedGCTime) {
-  assert(_last_gc_stat != NULL && _current_gc_stat != NULL, "Just checking");
+  // Inactive memory managers (concurrent in G1 legacy mode) will not
be initialized.
+  if (_last_gc_stat == NULL && _current_gc_stat == NULL) return;
+

 void GCMemoryManager::gc_end(bool recordPostGCUsage,
                              bool recordAccumulatedGCTime,
                              bool recordGCEndTime, bool countCollection,
                              GCCause::Cause cause,
                              bool allMemoryPoolsAffected) {
+  if (_last_gc_stat == NULL && _current_gc_stat == NULL) return;
+

Because they are only for handling the special case for
g1mm()->conc_memory_manager(), it is probably better not to change
memoryManager.cpp, but let TraceConcMemoryManagerStats handle them.
I was considering if we can make the calls to gc_begin() and gc_end()
conditional on G1UseLegacyMonitoring, but C++ does not allow complete
overriding of the base class's destructor ~TraceMemoryManagerStats(),
which calls GCMemoryManager::gc_end().

One option is to keep the code as-is, but I recommend adding
assertions that the two branches can only be taken when
G1UseLegacyMonitoring && this == g1mm()->conc_memory_manager().
The other option is to implement TraceConcMemoryManagerStats
independent of TraceMemoryManagerStats, so it can have a destructor
that conditionally calls GCMemoryManager::gc_end().

----------------------------File Separator----------------------------------
In http://cr.openjdk.java.net/~phh/8196989/webrev.04/test/hotspot/jtreg/gc/g1/mixedgc/TestOldGenCollectionUsage.java.udiff.html

-        if (newCollectionCount <= collectionCount) {
+        if (useLegacyMonitoring) {
+            if (newCollectionCount <= youngCollectionCount) {
             throw new RuntimeException("No new collection");
         }
-        if (newCollectionTime <= collectionTime) {
-            throw new RuntimeException("Collector has not run some more");
+        } else {
+            if (newCollectionCount <= 0) {
+                throw new RuntimeException("Mixed collection count <= 0");
+            }
+            if (newCollectionTime <= 0) {
+                throw new RuntimeException("Mixed collector did not run");
+            }
         }

It seems under the useLegacyMonitoring==true branch, the check for
"newCollectionTime <= collectionTime" was removed.
In addition, I think this test add a check that youngCollector and
mixedCollector point to the same instance if
useLegacyMonitoring==true.

----------------------------File Separator----------------------------------
In http://cr.openjdk.java.net/~phh/8196989/webrev.04/test/jdk/jdk/jfr/event/gc/stacktrace/AllocationStackTrace.java.udiff.html

     private static GarbageCollectorMXBean
garbageCollectorMXBean(String name) throws Exception {
         MBeanServer server = ManagementFactory.getPlatformMBeanServer();
-        GarbageCollectorMXBean bean = ManagementFactory.newPlatformMXBeanProxy(
-                server, "java.lang:type=GarbageCollector,name=" +
name, GarbageCollectorMXBean.class);
+        GarbageCollectorMXBean bean;
+        try {
+            bean = ManagementFactory.newPlatformMXBeanProxy(server,
+
"java.lang:type=GarbageCollector,name=" + name,
+
GarbageCollectorMXBean.class);
+        } catch (IllegalArgumentException e) {
+            bean = null;
+        }
+        return bean;
+    }
+
+    private static GarbageCollectorMXBean
g1YoungGarbageCollectorMXBean() throws Exception {
+        GarbageCollectorMXBean bean = garbageCollectorMXBean("G1
Young Generation");
+        if (bean == null) {
+            bean = garbageCollectorMXBean("G1 Young");
+        }
+        return bean;
+    }
+
+    private static GarbageCollectorMXBean
g1FullGarbageCollectorMXBean() throws Exception {
+        GarbageCollectorMXBean bean = garbageCollectorMXBean("G1 Old
Generation");
+        if (bean == null) {
+            bean = garbageCollectorMXBean("G1 Full");
+        }
         return bean;
     }

It is probably better to add the LegacyMonitoring checker class to
this file and use LegacyMonitoring.use() to determine the appropriate
name of the MXBeans.
Catching IllegalArgumentException and retrying with a different name
is like using exception for control flow.

Thanks,
Man

On Sun, Oct 21, 2018 at 3:08 PM David Holmes <david.holmes at oracle.com> wrote:
>
> Hi Paul,
>
> On 20/10/2018 2:05 AM, Hohensee, Paul wrote:
> > If we put the flag into deprecation, I’d like to keep it for a year so
> > people have time to change their monitoring code (one release to change
> > their code, and another to run with their new code), which would be two
> > releases. I don’t know how the deprecation process works either. Note
> > that if/when this gets backported to jdk8u and/or jdk11u, there’s no
> > mechanism there to obsolete a flag.
>
> First the new flag has to be added to the CSR to get approval to be
> added. It would be quite strange to add a new flag and deprecate it at
> the same time - not completely out of the question given its legacy
> compatibility nature, but still odd. So I'd suggest not initially
> deprecating it but rather file a new bug and CSR to deprecate in 13,
> obsolete in 14 and expire in 15. That gives you 12 and 13 where the flag
> is active - though you'll only get a deprecation warning in 13. The
> obsoletion in 14 will require new bug, but not CSR. The expiration is
> normally handled en-masse when we bump the JDK release version.
>
> For 8u the deprecation process is more manual. You need to add an
> explicit check and warning in arguments.cpp, then when you're ready to
> obsolete it add it to the obsolete flag table and remove the deprecation
> warning.
>
> Cheers,
> David
> -----
>
>
> > Discovered an issue with the
> > jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java
> > test, new new webrev at
> >
> > http://cr.openjdk.java.net/~phh/8196989/webrev.04/
> > <http://cr.openjdk.java.net/%7Ephh/8196989/webrev.04/>
> >
> > Paul
> >
> > *From: *JC Beyler <jcbeyler at google.com>
> > *Date: *Thursday, October 18, 2018 at 10:47 PM
> > *To: *"Hohensee, Paul" <hohensee at amazon.com>
> > *Cc: *"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
> >
> > Hi Paul,
> >
> > Looks much better to me. The other question I have is about the legacy
> > mode. I understand why, from a tool's perspective, having a legacy mode
> > is practical. By doing it this way, we are ensuring we don't break any
> > tools (or at least they can use a flag to be "unbroken") and give time
> > to migrate. This also provides an easier means to backport this fix to
> > older JDKs because now the legacy mode can be used to not break anything
> > and yet provide a means to migrate to a more sane vision of G1 collector
> > definitions.
> >
> > Should the flag perhaps be automatically put in deprecation and then we
> > can mark it as obsolete for JDK13? That would give a limited time for a
> > flag but again I'm not sure this is really done?
> >
> > Or is the plan to keep the flag for a given number of versions, try out
> > these new pools and ensure they provide what we need?
> >
> > Thanks!
> >
> > Jc
> >
> > On Thu, Oct 18, 2018 at 3:18 PM Hohensee, Paul <hohensee at amazon.com
> > <mailto:hohensee at amazon.com>> wrote:
> >
> >     Thanks for your review, JC.  New webrev:
> >     http://cr.openjdk.java.net/~phh/8196989/webrev.03/
> >     <http://cr.openjdk.java.net/%7Ephh/8196989/webrev.03/>
> >
> >     I updated the copyright date in memoryService.hpp because I forgot
> >     to do so in the patch for
> >     https://bugs.openjdk.java.net/browse/JDK-8195115. Thomas asked me to
> >     fix in it a separate CR, so I’ve reverted it. Ditto the #include
> >     fixes in g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp. At
> >     one point during development, clang complained about the latter, but
> >     no longer does.
> >
> >     The ‘break’ on the same line as the ‘}’ was in the original version,
> >     but I’ve moved it. :)
> >
> >     The comment is indeed a bit opaque. I changed it to:
> >
> >              // Only check heap pools that support a usage threshold.
> >
> >              // This is typically only the old generation space
> >
> >              // since the other spaces are expected to get filled up.
> >
> >              if (p.getType() == MemoryType.HEAP &&
> >
> >                 p.isUsageThresholdSupported()) {
> >
> >                     // In all collectors except G1, only the old
> >     generation supports a
> >
> >                      // usage threshold. The G1 legacy mode "G1 Old Gen"
> >     also does. In
> >
> >                      // G1 default mode, both the old space ("G1 Old
> >     Space": it's not
> >
> >                      // really a generation in the non-G1 collector
> >     sense) and the
> >
> >                      // humongous space ("G1 Humongous Space"), support
> >     a usage threshold.
> >
> >                      // So, the following condition is true for all
> >     non-G1 old generations,
> >
> >                      // for the G1 legacy old gen, and for the G1
> >     default humongous space.
> >
> >                     // It is not true for the G1 default old gen.
> >
> >                      //
> >
> >                      // We're allocating humongous objects in this test,
> >     so the G1 default
> >
> >                      // mode "G1 Old Space" occupancy doesn't change,
> >     because humongous
> >
> >                      // objects are allocated in the "G1 Humongous
> >     Space". If we allowed
> >
> >                      // the G1 default mode "G1 Old Space", notification
> >     would never
> >
> >                      // happen because no objects are allocated there.
> >
> >                     if (!p.getName().equals("G1 Old Space")) {
> >
> >     Finally, the G1MonitoringScope constructor now does a better job of
> >     selecting a memory manager.
> >
> >     Paul
> >
> >     *From: *JC Beyler <jcbeyler at google.com <mailto:jcbeyler at google.com>>
> >     *Date: *Wednesday, October 17, 2018 at 4:47 PM
> >     *To: *"Hohensee, Paul" <hohensee at amazon.com
> >     <mailto:hohensee at amazon.com>>
> >     *Cc: *"hotspot-gc-dev at openjdk.java.net
> >     <mailto:hotspot-gc-dev at openjdk.java.net>"
> >     <hotspot-gc-dev at openjdk.java.net
> >     <mailto:hotspot-gc-dev at openjdk.java.net>>,
> >     "serviceability-dev at openjdk.java.net
> >     <mailto:serviceability-dev at openjdk.java.net>"
> >     <serviceability-dev at openjdk.java.net
> >     <mailto:serviceability-dev at openjdk.java.net>>
> >     *Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and
> >     GarbageCollector MXBean definitions
> >
> >     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
> >     <http://cr.openjdk.java.net/%7Ephh/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
> >     <http://cr.openjdk.java.net/%7Ephh/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
> >     <http://cr.openjdk.java.net/%7Ephh/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
> >     <http://cr.openjdk.java.net/%7Ephh/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
> >     <mailto:hohensee at amazon.com>> wrote:
> >
> >         Ping.
> >
> >         *From: *serviceability-dev
> >         <serviceability-dev-bounces at openjdk.java.net
> >         <mailto:serviceability-dev-bounces at openjdk.java.net>> on behalf
> >         of "Hohensee, Paul" <hohensee at amazon.com
> >         <mailto:hohensee at amazon.com>>
> >         *Date: *Thursday, October 11, 2018 at 6:46 PM
> >         *To: *"hotspot-gc-dev at openjdk.java.net
> >         <mailto:hotspot-gc-dev at openjdk.java.net>"
> >         <hotspot-gc-dev at openjdk.java.net
> >         <mailto:hotspot-gc-dev at openjdk.java.net>>,
> >         "serviceability-dev at openjdk.java.net
> >         <mailto:serviceability-dev at openjdk.java.net>"
> >         <serviceability-dev at openjdk.java.net
> >         <mailto: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
> >         <mailto:serviceability-dev-bounces at openjdk.java.net>> on behalf
> >         of "Hohensee, Paul" <hohensee at amazon.com
> >         <mailto:hohensee at amazon.com>>
> >         *Date: *Monday, October 8, 2018 at 7:50 PM
> >         *To: *"hotspot-gc-dev at openjdk.java.net
> >         <mailto:hotspot-gc-dev at openjdk.java.net>"
> >         <hotspot-gc-dev at openjdk.java.net
> >         <mailto:hotspot-gc-dev at openjdk.java.net>>,
> >         "serviceability-dev at openjdk.java.net
> >         <mailto:serviceability-dev at openjdk.java.net>"
> >         <serviceability-dev at openjdk.java.net
> >         <mailto: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/
> >         <http://cr.openjdk.java.net/%7Ephh/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 3^rd 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
> >
> >
> > --
> >
> > Thanks,
> >
> > Jc
> >


More information about the hotspot-gc-dev mailing list