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

[Erik Österlund]

On Tue, 9 Apr 2024 16:12:05 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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.

I thought the comment about "avoid dropping a concurrent request" was rather talking about the flag being concurrently set from false to true (in record_needs_cleanup), but then immediately getting cleared to false here due to the lack of atomics, leading to the cleanup request being essentially ignored and handled as done, even though nothing was done.

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

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