RFR: 8329488: Move OopStorage code from safepoint cleanup and remove safepoint cleanup code [v3]

[Coleen Phillimore]

On Wed, 10 Apr 2024 05:19:24 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>>> The only reason for the store-release is because we use load-acquire and it's consistent.
>> 
>> So why do we use load-acquire? That is typically to pair with a store-release. We only need release semantics if someone seeing this store must see previous stores as well.
>
> Ah, okay. Then this seems fine. Maybe tweak or remove the comment to reflect this conversation. It seems to be saying the opposite of what it is doing.

The reason we have load_acquire is to match the release_store here. This can be running concurrently with the ServiceThread checking whether cleanup is needed.  This first flag says this OopStorage needs cleanup, the second is the global.  It seems like the order is important.  Therefore, we use all the load_acquire/release_store operations for safety and consistency, and to avoid future head scratching.

// Record that cleanup is needed, without notifying the Service thread, because
// we can't lock the Service_lock.  Used by release().
void OopStorage::record_needs_cleanup() {
  // Set local flag first, else ServiceThread could wake up and miss
  // the request.
  Atomic::release_store(&_needs_cleanup, true);
  Atomic::release_store_fence(&needs_cleanup_requested, true);
}

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1559390909