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

[Paul Hohensee]

On Fri, 5 May 2023 16:43:10 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
>
> 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`?

Removed.

> src/hotspot/share/services/management.cpp line 2115:
> 
>> 2113:       result += size;
>> 2114:     }
>> 2115:     return result + ThreadService::exited_allocated_bytes();;
> 
> Double `;;`.

Fixed.

> 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.

Fixed.

> 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.

Fixed.

> 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?

Fixed.

> 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`.

Fixed.

> 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(...)`?

Yes. Done.

> 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`?

I kept "sizes" since it's use before isn't really "before". I renamed sizes1 to afterSizes.

> 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.

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514932
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186515039
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513534
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513639
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513735
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513794
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514728
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514463
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514571