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

[Kim Barrett]

On Tue, 9 Apr 2024 15:42:50 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Specify meaning of ServiceThreadCleanupInterval.
>
> src/hotspot/share/gc/shared/oopStorage.cpp line 910:
> 
>> 908:     // Set the request flag false and return its old value.
>> 909:     // Needs to be atomic to avoid dropping a concurrent request.
>> 910:     Atomic::release_store(&needs_cleanup_requested, false);
> 
> The comment above seems to imply that needs_cleanup_requested can be set to true atomically and that we therefore have to use Atomic::xchg to make sure we don't drop a concurrent request for cleanup. Now the code has been changed to a release_store instead. If the comment is still right (I think it is?), then the code shouldchange back to xchg. Otherwise the comment should be updated to describe why we don't need xchg any longer, I think.

That comment is a left-over from the old mechanism, and is no longer true.  This is the only place
that sets it false (and is holding Service_lock so there can't be concurrent setters to false), and knows
it is true (from the earlier test).  The old protocol with the safepoint cleanup trigger was more complex,
and this function didn't have the test of the flag.  We probably don't even need a "release" here.

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

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