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

[Coleen Phillimore]

On Sat, 6 Apr 2024 12:52:10 GMT, Kim Barrett <kbarrett 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.
>
> 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."

I just changed notification to cleanup.  We already say the ServiceThread lots of places.

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

that looks better.

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

oh yes, that should be load_acquire to match.

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

I deleted the sentence.  It wasn't clear what it meant anyway.  There is a sentence about notification in the comment above the function but I think that's trying to say why this doesn't do a notification like all other ServiceThread cleanups, so is still saying something that might be useful.

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

Ok, I see.  The deferred updates may create an empty block which can then be cleaned up here? The suggested comment seems to make sense.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1555866403
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1555867392
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1555868403
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1555874533
PR Review Comment: https://git.openjdk.org/jdk/pull/18375#discussion_r1555876097