Discovered an issue with the
jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java
test, new new webrev at
http://cr.openjdk.java.net/~phh/8196989/webrev.04/
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.04/>
Paul
*From: *JC Beyler <jcbey...@google.com>
*Date: *Thursday, October 18, 2018 at 10:47 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>
*Cc: *"hotspot-gc-...@openjdk.java.net"
<hotspot-gc-...@openjdk.java.net>, "serviceability-dev@openjdk.java.net"
<serviceability-dev@openjdk.java.net>
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector
MXBean definitions
Hi Paul,
Looks much better to me. The other question I have is about the legacy
mode. I understand why, from a tool's perspective, having a legacy mode
is practical. By doing it this way, we are ensuring we don't break any
tools (or at least they can use a flag to be "unbroken") and give time
to migrate. This also provides an easier means to backport this fix to
older JDKs because now the legacy mode can be used to not break anything
and yet provide a means to migrate to a more sane vision of G1 collector
definitions.
Should the flag perhaps be automatically put in deprecation and then we
can mark it as obsolete for JDK13? That would give a limited time for a
flag but again I'm not sure this is really done?
Or is the plan to keep the flag for a given number of versions, try out
these new pools and ensure they provide what we need?
Thanks!
Jc
On Thu, Oct 18, 2018 at 3:18 PM Hohensee, Paul <hohen...@amazon.com
<mailto:hohen...@amazon.com>> wrote:
Thanks for your review, JC. New webrev:
http://cr.openjdk.java.net/~phh/8196989/webrev.03/
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.03/>
I updated the copyright date in memoryService.hpp because I forgot
to do so in the patch for
https://bugs.openjdk.java.net/browse/JDK-8195115. Thomas asked me to
fix in it a separate CR, so I’ve reverted it. Ditto the #include
fixes in g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp. At
one point during development, clang complained about the latter, but
no longer does.
The ‘break’ on the same line as the ‘}’ was in the original version,
but I’ve moved it. :)
The comment is indeed a bit opaque. I changed it to:
// Only check heap pools that support a usage threshold.
// This is typically only the old generation space
// since the other spaces are expected to get filled up.
if (p.getType() == MemoryType.HEAP &&
p.isUsageThresholdSupported()) {
// In all collectors except G1, only the old
generation supports a
// usage threshold. The G1 legacy mode "G1 Old Gen"
also does. In
// G1 default mode, both the old space ("G1 Old
Space": it's not
// really a generation in the non-G1 collector
sense) and the
// humongous space ("G1 Humongous Space"), support
a usage threshold.
// So, the following condition is true for all
non-G1 old generations,
// for the G1 legacy old gen, and for the G1
default humongous space.
// It is not true for the G1 default old gen.
//
// We're allocating humongous objects in this test,
so the G1 default
// mode "G1 Old Space" occupancy doesn't change,
because humongous
// objects are allocated in the "G1 Humongous
Space". If we allowed
// the G1 default mode "G1 Old Space", notification
would never
// happen because no objects are allocated there.
if (!p.getName().equals("G1 Old Space")) {
Finally, the G1MonitoringScope constructor now does a better job of
selecting a memory manager.
Paul
*From: *JC Beyler <jcbey...@google.com <mailto:jcbey...@google.com>>
*Date: *Wednesday, October 17, 2018 at 4:47 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com
<mailto:hohen...@amazon.com>>
*Cc: *"hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>"
<hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>>,
"serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>"
<serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>>
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and
GarbageCollector MXBean definitions
Hi Paul,
I looked at this but it took time for me to "digest" it and I
haven't entirely gone through the real GC changes :)
My few remarks on the webrev itself are:
-
http://cr.openjdk.java.net/~phh/8196989/webrev.02/src/hotspot/share/services/memoryService.hpp.udiff.html
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.02/src/hotspot/share/services/memoryService.hpp.udiff.html>
- There is no need to change the copyright, right?
-
http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java.udiff.html
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.02/test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java.udiff.html>
- the break seems to be on the wrong line, no?
+ } break;
-
http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest.java.udiff.html>
and
http://cr.openjdk.java.net/~phh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/MemoryManagement.java.udiff.html
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.02/test/jdk/java/lang/management/MemoryMXBean/MemoryManagement.java.udiff.html>
+ // In G1, humongous objects are tracked in
the old space only in
+ // legacy monitoring mode. In default mode,
G1 tracks humongous
+ // objects in the humongous space, which
latter also supports a
+ // usage threshold. Since we're allocating
humongous objects in
+ // this test, in default mode the old space
doesn't change. For
+ // this test, we use the old space in
legacy mode (it's called
+ // "G1 Old Gen" and the humongous space in
default mode. If we
+ // used "G1 Old Space" in default mode,
notification would never
+ // happen.
-> latter seems ot be the wrong word or something is missing in that
sentence
-> the parenthesis is never closed (it's called.... is missing a )
somewhere
Thanks,
Jc
On Wed, Oct 17, 2018 at 1:18 PM Hohensee, Paul <hohen...@amazon.com
<mailto:hohen...@amazon.com>> wrote:
Ping.
*From: *serviceability-dev
<serviceability-dev-boun...@openjdk.java.net
<mailto:serviceability-dev-boun...@openjdk.java.net>> on behalf
of "Hohensee, Paul" <hohen...@amazon.com
<mailto:hohen...@amazon.com>>
*Date: *Thursday, October 11, 2018 at 6:46 PM
*To: *"hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>"
<hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>>,
"serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>"
<serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>>
*Subject: *Re: 8196989: Revamp G1 JMX MemoryPool and
GarbageCollector MXBean definitions
Any takers? :)
*From: *serviceability-dev
<serviceability-dev-boun...@openjdk.java.net
<mailto:serviceability-dev-boun...@openjdk.java.net>> on behalf
of "Hohensee, Paul" <hohen...@amazon.com
<mailto:hohen...@amazon.com>>
*Date: *Monday, October 8, 2018 at 7:50 PM
*To: *"hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>"
<hotspot-gc-...@openjdk.java.net
<mailto:hotspot-gc-...@openjdk.java.net>>,
"serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>"
<serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>>
*Subject: *RFR: 8196989: Revamp G1 JMX MemoryPool and
GarbageCollector MXBean definitions
Bug: https://bugs.openjdk.java.net/browse/JDK-8196989
CSR: https://bugs.openjdk.java.net/browse/JDK-8196991
Webrev: http://cr.openjdk.java.net/~phh/8196989/webrev.02/
<http://cr.openjdk.java.net/%7Ephh/8196989/webrev.02/>
As requested, I split the jstat counter update off from the
MXBean update. This is the MXBean update. The jstat counter RFE
is https://bugs.openjdk.java.net/browse/JDK-8210965 and its CSR
is https://bugs.openjdk.java.net/browse/JDK-8210966.
The MXBean CSR is in draft state, I’d greatly appreciate review
and sign-off.
It’s been suggested that we add another pool to represent the
free region set, but doing so would be incompatible with
existing MXBean use invariants for all GCs. These are:
1. The sum of the pools’ MemoryUsage.max properties is the
total reserved heap size.
2. The sum of the pools’ MemoryUsage.committed properties is
the total committed size.
3. The sum of the pools’ MemoryUsage.used properties is the
total size of the memory containing objects, live and
dead-and-yet-to-be-collected, as the case might be, plus
intentional gaps between them.
4. The total free space is (sum of the max properties – sum of
the used properties).
5. The total uncommitted space is (sum of the max properties –
sum of the committed properties).
6. The total committed free space is (2) – (3).
To keep invariants 1, 2 and 3, the free region pool’s “max”
property should be “undefined” (i.e., -1). The intuitive, to me,
“used” property value would be the total free space, but that
would violate invariant 4 above. Defining the “committed”
property as the total committed free space would violate
invariants 2 and 6.
The patch passes the submit repo, hotspot tier1, and,
separately, the serviceability, jfr, and gc jtreg tests. I’m
uncertain how to construct a test that checks for valid MXBean
content: the existing tests don’t. Any such test will be fragile
due to possible future Hotspot changes that affect the values,
and to run-to-run variability. I’ve done by-hand comparisons
between the old and new MXBean content using the SwingSet2 demo,
including using App CDS, and the numbers look reasonable.
The guts of the change are in
G1MonitoringSupport::recalculate_sizes,
initialize_serviceability, memory_managers, memory_pools, and
G1MonitoringScope. I also defined TraceConcMemoryManagerStats to
track the concurrent cycle in a way analogous to
TraceCMSMemoryManagerStats. The changes to the includes in
g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp are to
satisfy compiler complaints. I changed the 3^rd argument to the
G1MonitoringScope constructor to a mixed_gc flag, and use
accessor methods instead of direct field accesses when accessor
methods exist. I believe I’ve minimized the latter. I updated
the copyright date to 2018 in memoryService.hpp because I
neglected to do so in my previous G1 MXBean patch.
Thanks,
Paul
--
Thanks,
Jc
--
Thanks,
Jc
