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

[Paul Hohensee]

On Fri, 19 May 2023 03:08:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8304074: Need acquire/release in incr_exited_allocated_bytes
>
> src/hotspot/share/services/threadService.hpp line 117:
> 
>> 115:     // store to _exited_allocated_bytes above this one.
>> 116:     jlong old = Atomic::load_acquire(&_exited_allocated_bytes);
>> 117:     Atomic::release_store(&_exited_allocated_bytes, old + size);
> 
> First yes Atomic::load is not needed here due to the lock, it is needed in the accessor above - sorry that wasn't clear. The Atomic store is needed of course.
> 
> But the load_acquire is definitely not needed as there is an implicit acquire when the lock is acquired, so any load of the field here must see the last value written under the lock. The release_store is also not needed because a release is for ensuring visibility of previous stores of which there are none, and has no affect on the visibility of the store done by the `release_store`.
> 
> So lets imagine a really weakly-ordered piece of hardware where all of the writing threads are completely properly synchronized by the use of the lock (else the hw is broken), but a lock-free reader can potentially see any of those writes out-of-order.  I think the only way to guarantee seeing the most recent write would be to issue a full fence() before the load. But I would be very surprised if any of our hardware actually operates in such a fashion.

Thanks for the explanation. I reverted the acquire/release and the atomic load in incr_exited_allocated_bytes and added an atomic load in exited_allocated_bytes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1198986781