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.hol...@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 <jcbey...@google.com> > > *Date: *Thursday, October 18, 2018 at 10:47 PM > > *To: *"Hohensee, Paul" <hohen...@amazon.com> > > *Cc: *"hotspot-gc-...@openjdk.java.net" > > <hotspot-gc-...@openjdk.java.net>, "serviceability-dev@openjdk.java.net" > > <serviceability-dev@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 <hohen...@amazon.com > > <mailto:hohen...@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 <jcbey...@google.com <mailto:jcbey...@google.com>> > > *Date: *Wednesday, October 17, 2018 at 4:47 PM > > *To: *"Hohensee, Paul" <hohen...@amazon.com > > <mailto:hohen...@amazon.com>> > > *Cc: *"hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>" > > <hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>>, > > "serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@openjdk.java.net>" > > <serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@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 <hohen...@amazon.com > > <mailto:hohen...@amazon.com>> wrote: > > > > Ping. > > > > *From: *serviceability-dev > > <serviceability-dev-boun...@openjdk.java.net > > <mailto:serviceability-dev-boun...@openjdk.java.net>> on behalf > > of "Hohensee, Paul" <hohen...@amazon.com > > <mailto:hohen...@amazon.com>> > > *Date: *Thursday, October 11, 2018 at 6:46 PM > > *To: *"hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>" > > <hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>>, > > "serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@openjdk.java.net>" > > <serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@openjdk.java.net>> > > *Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and > > GarbageCollector MXBean definitions > > > > Any takers? :) > > > > *From: *serviceability-dev > > <serviceability-dev-boun...@openjdk.java.net > > <mailto:serviceability-dev-boun...@openjdk.java.net>> on behalf > > of "Hohensee, Paul" <hohen...@amazon.com > > <mailto:hohen...@amazon.com>> > > *Date: *Monday, October 8, 2018 at 7:50 PM > > *To: *"hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>" > > <hotspot-gc-...@openjdk.java.net > > <mailto:hotspot-gc-...@openjdk.java.net>>, > > "serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@openjdk.java.net>" > > <serviceability-dev@openjdk.java.net > > <mailto:serviceability-dev@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 > >
