RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM

[Aleksey Shipilev]

On Thu, 4 May 2023 19:54:57 GMT, Paul Hohensee <phh at openjdk.org> wrote:

> Please review this addition to com.sun.management.ThreadMXBean that returns the total number of bytes allocated on the Java heap since JVM launch by both terminated and live threads.
> 
> Because this PR adds a new interface method, I've updated the JMM_VERSION to 4, but would be happy to update it to 3_1 instead.

Some comments follow.

src/hotspot/share/include/jmm.h line 55:

> 53:   JMM_VERSION_2   = 0x20020000, // JDK 10
> 54:   JMM_VERSION_3   = 0x20030000, // JDK 14
> 55:   JMM_VERSION_3_0 = 0x20030000,

What's `JMM_VERSION_3_0`?

src/hotspot/share/services/management.cpp line 2115:

> 2113:       result += size;
> 2114:     }
> 2115:     return result + ThreadService::exited_allocated_bytes();;

Double `;;`.

src/hotspot/share/services/threadService.hpp line 111:

> 109:   static jlong exited_allocated_bytes()       { return _exited_allocated_bytes; }
> 110:   static void incr_exited_allocated_bytes(jlong size) {
> 111:     Atomic::add(&_exited_allocated_bytes, size);

`Atomic::add(&_exited_allocated_bytes, size, memory_order_relaxed);`, please. No need for overly-strict memory effects for this counter.

src/java.management/share/classes/sun/management/ThreadImpl.java line 535:

> 533:     private static native long getThreadAllocatedMemory0(long id);
> 534:     private static native void getThreadAllocatedMemory1(long[] ids, long[] result);
> 535:     private static native long getThreadAllocatedMemory2();

We can call this one `getAllThreadAllocatedMemory`, which obviates the need for `2` as the suffix.

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java line 159:

> 157:      *
> 158:      * @return an approximation of the total memory allocated, in bytes, in
> 159:      * heap memory for the current thread,

I am not sure if typos changes in the public API requires a CSR (albeit trivial one). Maybe skip these updates?

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 221:

> 219:         // baseline should be positive
> 220:         Thread curThread = Thread.currentThread();
> 221:         long cumulative_size = mbean.getAllThreadAllocatedBytes();

Java style for variables is camel-case, `cumulativeSize`.

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 286:

> 284:     }
> 285: 
> 286:     private static long checkResult(Thread curThread,

There is another `checkResult` below? Should they be replaced by a single method?

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 377:

> 375:                 throw new RuntimeException(getName() +
> 376:                     " ThreadAllocatedBytes before = " + size1 +
> 377:                     " > ThreadAllocatedBytes after = " + size2);

Is this replaceable with `checkResult(...)`?

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemoryArray.java line 120:

> 118:         long[] sizes1 = mbean.getThreadAllocatedBytes(ids);
> 119:         for (int i = 0; i < NUM_THREADS; i++) {
> 120:             checkResult(threads[i], sizes[i], sizes1[i]);

Since we are cleaning up the test anyway, can we / should we rename `sizes` -> `before`, `size1` -> `after`?

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemoryArray.java line 164:

> 162: 
> 163:     private static void checkResult(Thread curThread,
> 164:                                     long prev_size, long curr_size) {

camelCase arguments.

-------------

PR Review: https://git.openjdk.org/jdk/pull/13814#pullrequestreview-1415057714
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186299709
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186309343
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186260976
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186261881
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186263383
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186265606
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186277116
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186275693
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186275048
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186275416