RFR: 8329488: Move OopStorage code from safepoint cleanup and remove safepoint cleanup code [v3]
Erik Österlund
eosterlund at openjdk.org
Tue Apr 9 15:46:00 UTC 2024
On Tue, 9 Apr 2024 13:56:28 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
>> This patch gives the ServiceThread a periodic wakeup (same as GuaranteedSafepointInterval) to check if it needs to clean out OopStorage blocks, and move the triggering of this cleaning out of the safepoint cleanup tasks. Since ICBuffer, StringTable and SymbolTable rehashing have moved, there's nothing that actually triggers the nop safepoint to do cleaning (except SafepointALot), so the OopStorage cleanup won't be triggered.
>>
>> With moving all of these out of the safepoint cleanup tasks, we can remove the code that sets up multiple threads to do safepoint cleanup. We can also remove the JFR events and logging that times safepoint cleanup, and a logging test.
>>
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
> Specify meaning of ServiceThreadCleanupInterval.
I think I spotted a tiny issue otherwise this looks great!
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.
-------------
Changes requested by eosterlund (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18375#pullrequestreview-1989431987
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1557880835
More information about the hotspot-dev
mailing list