RFR: 8329488: Move OopStorage code from safepoint cleanup and remove safepoint cleanup code

[Kim Barrett]

On Tue, 19 Mar 2024 12:19:44 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.

Mostly looks good, with some remaining tidying up to do.

src/hotspot/share/gc/shared/oopStorage.cpp line 895:

> 893: 
> 894: // Time after which a notification can be made.
> 895: static jlong cleanup_permit_time = 0;

This mechanism no longer involves notification, so comment needs to be updated. Maybe
"Time when ServiceThread is next permitted to do cleanup."

src/hotspot/share/gc/shared/oopStorage.cpp line 897:

> 895: static jlong cleanup_permit_time = 0;
> 896: 
> 897: // Minimum time since last ServiceThread check before cleanup is permitted.

Maybe "Minimum time between ServiceThread cleanups."

src/hotspot/share/gc/shared/oopStorage.cpp line 904:

> 902:   assert_lock_strong(Service_lock);
> 903: 
> 904:   if (Atomic::load(&needs_cleanup_requested) && os::javaTimeNanos() > cleanup_permit_time) {

Should be Atomic::load_acquire, matching release_store in record_needs_cleanup.

src/hotspot/share/gc/shared/oopStorage.cpp line 920:

> 918: void OopStorage::record_needs_cleanup() {
> 919:   // Set local flag first, else ServiceThread could wake up and miss
> 920:   // the request.  This order may instead (rarely) unnecessarily notify.

There's no longer any notification involved.  However, there is still the (rare) possibility that the ServiceThread
will uselessly run.  It might have already been doing cleanup and processed the block just added.  If no new
cleanup work gets added before the next ServiceThread cleanup time, it will attempt cleanup (because of the
flag(s) being set), and find nothing to do.  That's okay.  Or just delete the sentence about unnecessary notify.

src/hotspot/share/gc/shared/oopStorage.cpp line 928:

> 926:   // Service thread might have oopstorage work, but not for this object.
> 927:   // Check for deferred updates even though that's not a ServiceThread
> 928:   // cleanup; since we're here, we might as well process them.

That's not what's really going on here.  Replace the comment with
"But check for deferred updates, which might provide cleanup work."
Also, in previous unchanged line, s/Service thread/ServiceThread/

src/hotspot/share/gc/shared/oopStorage.cpp line 988:

> 986:   // Exceeded work limit or can't delete last block.  This will
> 987:   // cause the ServiceThread to loop, giving other subtasks an
> 988:   // opportunity to run too. There's no need for a notification,

With the changes to `has_cleanup_work_and_reset` this no longer causes the ServiceThread to loop.
Instead it requests cleanup at the next scheduled time for the ServiceThread to do so.  And there's no
longer ever any notification, so the final sentence needs some adjustment.

src/hotspot/share/runtime/serviceThread.cpp line 130:

> 128:              ) == 0) {
> 129:         // Wait until notified that there is some work to do or timer expires.
> 130:         // OopStorage work needs to be done at periodic intervals.

Rather than calling out OopStorage here, maybe just say some cleanup requests don't notify the
ServiceThread, instead relying on it to run periodically.  After this change we might want to audit
other cleanup requests and decide if they actually need to notify the ServiceThread in order to get a
more prompt response, or could just wait for the next periodic wakeup.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18375#pullrequestreview-1984494587
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554582541
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554582637
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554583069
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554584073
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554584508
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554585100
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1554585933