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 <hohen...@amazon.com> wrote: > Ping. > > > > *From: *serviceability-dev <serviceability-dev-boun...@openjdk.java.net> > on behalf of "Hohensee, Paul" <hohen...@amazon.com> > *Date: *Thursday, October 11, 2018 at 6:46 PM > *To: *"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 > > > > Any takers? :) > > > > *From: *serviceability-dev <serviceability-dev-boun...@openjdk.java.net> > on behalf of "Hohensee, Paul" <hohen...@amazon.com> > *Date: *Monday, October 8, 2018 at 7:50 PM > *To: *"hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>, > "serviceability-dev@openjdk.java.net" <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/ > > > > 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
