Discussion:
RFR: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions
(too old to reply)
Hohensee, Paul
2018-10-08 23:49:33 UTC
Permalink
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
Hohensee, Paul
2018-10-11 22:45:16 UTC
Permalink
Any takers? :)

From: serviceability-dev <serviceability-dev-***@openjdk.java.net> on behalf of "Hohensee, Paul" <***@amazon.com>
Date: Monday, October 8, 2018 at 7:50 PM
To: "hotspot-gc-***@openjdk.java.net" <hotspot-gc-***@openjdk.java.net>, "serviceability-***@openjdk.java.net" <serviceability-***@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
Hohensee, Paul
2018-10-17 20:17:25 UTC
Permalink
Ping.

From: serviceability-dev <serviceability-dev-***@openjdk.java.net> on behalf of "Hohensee, Paul" <***@amazon.com>
Date: Thursday, October 11, 2018 at 6:46 PM
To: "hotspot-gc-***@openjdk.java.net" <hotspot-gc-***@openjdk.java.net>, "serviceability-***@openjdk.java.net" <serviceability-***@openjdk.java.net>
Subject: Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

Any takers? :)

From: serviceability-dev <serviceability-dev-***@openjdk.java.net> on behalf of "Hohensee, Paul" <***@amazon.com>
Date: Monday, October 8, 2018 at 7:50 PM
To: "hotspot-gc-***@openjdk.java.net" <hotspot-gc-***@openjdk.java.net>, "serviceability-***@openjdk.java.net" <serviceability-***@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
JC Beyler
2018-10-17 20:46:30 UTC
Permalink
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
Ping.
*Date: *Thursday, October 11, 2018 at 6:46 PM
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector
MXBean definitions
Any takers? :)
*Date: *Monday, October 8, 2018 at 7:50 PM
*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
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
Hohensee, Paul
2018-10-18 22:18:35 UTC
Permalink
Thanks for your review, JC. New webrev: http://cr.openjdk.java.net/~phh/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 <***@google.com>
Date: Wednesday, October 17, 2018 at 4:47 PM
To: "Hohensee, Paul" <***@amazon.com>
Cc: "hotspot-gc-***@openjdk.java.net" <hotspot-gc-***@openjdk.java.net>, "serviceability-***@openjdk.java.net" <serviceability-***@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
- 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 <***@amazon.com<mailto:***@amazon.com>> wrote:
Ping.

From: serviceability-dev <serviceability-dev-***@openjdk.java.net<mailto:serviceability-dev-***@openjdk.java.net>> on behalf of "Hohensee, Paul" <***@amazon.com<mailto:***@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-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>>
Subject: Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

Any takers? :)

From: serviceability-dev <serviceability-dev-***@openjdk.java.net<mailto:serviceability-dev-***@openjdk.java.net>> on behalf of "Hohensee, Paul" <***@amazon.com<mailto:***@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-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@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
JC Beyler
2018-10-19 02:45:59 UTC
Permalink
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
Post by Hohensee, Paul
http://cr.openjdk.java.net/~phh/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. :)
// 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
*Date: *Wednesday, October 17, 2018 at 4:47 PM
*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 :)
-
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
Ping.
*Date: *Thursday, October 11, 2018 at 6:46 PM
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector
MXBean definitions
Any takers? :)
*Date: *Monday, October 8, 2018 at 7:50 PM
*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
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
--
Thanks,
Jc
Hohensee, Paul
2018-10-19 16:05:09 UTC
Permalink
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.

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/

Paul

From: JC Beyler <***@google.com>
Date: Thursday, October 18, 2018 at 10:47 PM
To: "Hohensee, Paul" <***@amazon.com>
Cc: "hotspot-gc-***@openjdk.java.net" <hotspot-gc-***@openjdk.java.net>, "serviceability-***@openjdk.java.net" <serviceability-***@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 <***@amazon.com<mailto:***@amazon.com>> wrote:
Thanks for your review, JC. New webrev: http://cr.openjdk.java.net/~phh/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 <***@google.com<mailto:***@google.com>>
Date: Wednesday, October 17, 2018 at 4:47 PM
To: "Hohensee, Paul" <***@amazon.com<mailto:***@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-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@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
- 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 <***@amazon.com<mailto:***@amazon.com>> wrote:
Ping.

From: serviceability-dev <serviceability-dev-***@openjdk.java.net<mailto:serviceability-dev-***@openjdk.java.net>> on behalf of "Hohensee, Paul" <***@amazon.com<mailto:***@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-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>>
Subject: Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions

Any takers? :)

From: serviceability-dev <serviceability-dev-***@openjdk.java.net<mailto:serviceability-dev-***@openjdk.java.net>> on behalf of "Hohensee, Paul" <***@amazon.com<mailto:***@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-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@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
--
Thanks,
Jc
David Holmes
2018-10-21 22:07:57 UTC
Permalink
Hi Paul,
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
*Date: *Thursday, October 18, 2018 at 10:47 PM
*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
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. :)
        // 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
*Date: *Wednesday, October 17, 2018 at 4:47 PM
*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 :)
   -
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
Ping.
*From: *serviceability-dev
*Date: *Thursday, October 11, 2018 at 6:46 PM
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and
GarbageCollector MXBean definitions
Any takers? :)
*From: *serviceability-dev
*Date: *Monday, October 8, 2018 at 7:50 PM
*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
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
Man Cao
2018-10-23 21:37:20 UTC
Permalink
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
Post by JC Beyler
Hi Paul,
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
*Date: *Thursday, October 18, 2018 at 10:47 PM
*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
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. :)
// 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
*Date: *Wednesday, October 17, 2018 at 4:47 PM
*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 :)
-
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
Ping.
*From: *serviceability-dev
*Date: *Thursday, October 11, 2018 at 6:46 PM
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and
GarbageCollector MXBean definitions
Any takers? :)
*From: *serviceability-dev
*Date: *Monday, October 8, 2018 at 7:50 PM
*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
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
Thomas Schatzl
2018-10-23 13:37:43 UTC
Permalink
Hi Paul,

sorry for the long wait.
Post by Hohensee, Paul
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/
Note that I reviewed the latest change at

http://cr.openjdk.java.net/~phh/8196989/webrev.04/

Currently pushing this through our testing.

At that point the thread itself has moved to a question of a particular
detail, with too many indentations to the email text I wanted to
actually answer, so I answered to the original email. :)
Post by Hohensee, Paul
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.
I will look at it. As David already mentioned, it also needs to mention
the new flag regardless of whether introducing it as deprecated or not.
Post by Hohensee, Paul
It’s been suggested that we add another pool to represent the free
region set, but doing so would be incompatible with existing MXBean
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).
I think we discussed this before, but none of these so-called
invariants are specified anywhere and artifacts of the particular heap
layout and MXBean implementations for Serial GC/Parallel GC and CMS.

My understanding is that there are implementations out there that
assume just that - but those can just use the "legacy" mxbeans.

Currently, the MXBean values for G1 have been hacked to conform to this
scheme for "compatibility reasons" (e.g. eden regions max is -1; or
just look at the sorry code bending over backwards in
recalculate_sizes()). I would prefer to not carry on with this,
particularly with the new collectors (ZGC and Shenandoah) in the future
in mind, with a potential "young gen" going to have the same issues
(not sure about Shenandoah going to have something like this, but I
assume so for various reasons).

This change would be a good opportunity to do things correctly and
return values as the VM sees them (like proper max values for all
MXBeans), and avoid another round of having a "UseLegacyMemoryMXBeans2"
switch later.

Additionally most of these invariants do not even hold for the "old"
collectors if you look closely. One simple case is a GC occuring
between readings of the MemoryMXBean (with the known workaround retry
until you get consisten values instead of calling the right method),
and the issue related to JDK-8207200 (which *exactly* did rely on these
"invariants", and was broken because of that).

I also want to point out that the jstat counters already return heap
size as max capacity (which probably most of the MemoryMXBeans would
also return if fixed).

So overall I honestly do not like the scope of this change, i.e. this
half-fixing of G1 MemoryMXBeans for all these reasons (while the change
itself is good).

Changing the values returned by MemoryMXBeans would not decrease the
amount of information or quality you get, but rather improve both, and
make sure that future generational enhancements of collectors won't
have to bend backwards for broken code that assumes non-"invariants"
again.
Post by Hohensee, Paul
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.
My intuitive take on the values of the Free region space would be:
- used = 0, there are never objects in there, it's the *free* space
after all.
- committed = number of regions committed to Free regions
- max = current max heap size - other committed size, as this would
then also encompass the so-called "Not Available" Free regions in G1.
It would probably be too complicated and not relevant to the users to
have an extra MXBean for those.
Post by Hohensee, Paul
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.
- I am not sure about the reasons for the change in
G1CollectedHeap::post_initialize(). Did you experience some problems,
or is this some cleanup?

- the comment in g1Collected.hpp:1224 is misplaced, if you touch it,
please remove. The "surv rate groups" moved to G1Policy long ago.

- the two "Record start/end, but take no time" comments in
g1ConcurrentMark.cpp seem to not be very useful. The call below tells
you that exactly that happens, but not why, adding no information. I
would remove them.

- the comments at the top of g1MemoryPool.hpp and
g1MemoryPool.hpp:66-68 and 95-98 are nice but seem to mostly duplicate
the information in the comment in g1MonitoringSupport.hpp. Maybe they
could be merged a bit to avoid having too much duplicate information
around (and refer to it in g1MemoryPool).

- in the G1MonitoringSupport constructor, please avoid specifying
multiple member variable initializations in the same line using
additional formatting via spaces (I'm fine with additional newlines if
you want to group them). That is somewhat uncommon to read.

- g1MonitoringSupport.cpp:288: please remove the unused variable
old_regions_count.

- the comment in g1MonitoringSupport.hpp:277 needs to be made more
concise (thanks for fixing up lots of comments in this change!) by
adding something like:

"Values between different returned MemoryUsage may not be consistent
with each other."

- memoryManager.cpp:214/248: please add braces around the return
statement.

Rest of the changes seems good.

Thanks,
Thomas
Thomas Schatzl
2018-10-24 09:57:37 UTC
Permalink
Hi again,
Post by JC Beyler
Hi Paul,
sorry for the long wait.
Post by Hohensee, Paul
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/
Note that I reviewed the latest change at
http://cr.openjdk.java.net/~phh/8196989/webrev.04/
Currently pushing this through our testing.
There are several failures that I think can be directly attributed to
this change:

jdk tests (run jtreg from test/jdk)

com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotification
Test.java: "notifications have not been sent for java.lang:name=G1
Mixed, type=GarbageCollector"
com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotification
ContentTest.java: fails with a NullPointerException

I can reproduce these two locally when running without any additional
options. They seem to only occur with G1.

hotspot tests (run jtreg from test/hotspot)

java/lang/management/MemoryMXBean/MemoryTest.java: "Number of heap
pools = 3, but expected 5"

Note that the test may be called with other collectors than G1 from
outside, just not with ZGC as per the @requires tag, i.e. with
-vmoption:-XX:+UseParallelGC.

Thanks,
Thomas
Thomas Schatzl
2018-10-24 10:39:21 UTC
Permalink
Hi,
Post by Thomas Schatzl
Hi again,
Post by JC Beyler
Hi Paul,
sorry for the long wait.
Post by Hohensee, Paul
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/
Note that I reviewed the latest change at
http://cr.openjdk.java.net/~phh/8196989/webrev.04/
Currently pushing this through our testing.
There are several failures that I think can be directly attributed to
jdk tests (run jtreg from test/jdk)
com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificati
onTest.java: "notifications have not been sent for java.lang:name=G1
Mixed, type=GarbageCollector"
com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificati
onContentTest.java: fails with a NullPointerException
I can reproduce these two locally when running without any additional
options. They seem to only occur with G1.
hotspot tests (run jtreg from test/hotspot)
java/lang/management/MemoryMXBean/MemoryTest.java: "Number of heap
pools = 3, but expected 5"
Note that the test may be called with other collectors than G1 from
-vmoption:-XX:+UseParallelGC.
Also
vmTestbase/nsk/monitoring/MemoryNotificationInfo/from/from001/TestDescr
iption.java from the same location starts to time out with the changes.

Thanks,
Thomas

Loading...