On Thu, 18 May 2023 13:55:59 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.
>
> 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
Changes requested by dholmes (Reviewer).
src/hotspot/share/services/threadService.hpp line 109:
> 107: static jlong get_daemon_thread_count() { return
> _atomic_daemon_threads_count; }
> 108:
> 109: static jlong exited_allocated_bytes() { return
> _exited_allocated_bytes; }
This needs to be an atomic::load too .
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13814#pullrequestreview-1433711869
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1198498123
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1198500354
