On Thu, 4 May 2023 19:54:57 GMT, Paul Hohensee <p...@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
