On Fri, 5 May 2023 16:43:10 GMT, Aleksey Shipilev <sh...@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
