[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions
From: Man Cao <manc () google ! com>
Date: 2018-10-23 21:37:20
Message-ID: CA+w6HxY7LAD=R5Yi54N_bHPE2PR9KvkrZV6E7pLqq9YujMV1oA () mail ! gmail ! com
[Download RAW message or body]
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@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@google.com>
> > *Date: *Thursday, October 18, 2018 at 10:47 PM
> > *To: *"Hohensee, Paul" <hohensee@amazon.com>
> > *Cc: *"hotspot-gc-dev@openjdk.java.net"
> > <hotspot-gc-dev@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 <hohensee@amazon.com
> > <mailto:hohensee@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@google.com <mailto:jcbeyler@google.com>>
> > *Date: *Wednesday, October 17, 2018 at 4:47 PM
> > *To: *"Hohensee, Paul" <hohensee@amazon.com
> > <mailto:hohensee@amazon.com>>
> > *Cc: *"hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@openjdk.java.net>"
> > <hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@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 <hohensee@amazon.com
> > <mailto:hohensee@amazon.com>> wrote:
> >
> > Ping.
> >
> > *From: *serviceability-dev
> > <serviceability-dev-bounces@openjdk.java.net
> > <mailto:serviceability-dev-bounces@openjdk.java.net>> on behalf
> > of "Hohensee, Paul" <hohensee@amazon.com
> > <mailto:hohensee@amazon.com>>
> > *Date: *Thursday, October 11, 2018 at 6:46 PM
> > *To: *"hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@openjdk.java.net>"
> > <hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@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-bounces@openjdk.java.net
> > <mailto:serviceability-dev-bounces@openjdk.java.net>> on behalf
> > of "Hohensee, Paul" <hohensee@amazon.com
> > <mailto:hohensee@amazon.com>>
> > *Date: *Monday, October 8, 2018 at 7:50 PM
> > *To: *"hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@openjdk.java.net>"
> > <hotspot-gc-dev@openjdk.java.net
> > <mailto:hotspot-gc-dev@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
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic