On Fri, 19 May 2023 03:08:40 GMT, David Holmes <dhol...@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
