RFR: 8351444: Shenandoah: Class Unloading may encounter recycled oops [v3]

[William Kemper]

On Tue, 11 Mar 2025 15:58:23 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Trim extraneous comment
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 844:
> 
>> 842:   // during weak roots. Concurrent class unloading may access unmarked oops
>> 843:   // in trash regions.
>> 844:   return r->is_trash() && is_concurrent_weak_root_in_progress();
> 
> Pity to do this, but I understand the reason for it.
> 
> We should investigate if this window is unnecessarily large. I see currently we drop `WEAK_ROOTS` gc state in `ShenandoahHeap::concurrent_prepare_for_update_refs`. Should we drop the flag sooner, somewhere after concurrent class unloading? Can be done separately, if it snowballs into something more complicated.

Class unloading is the last thing we do before recycling trash regions. A region will be usable for allocation as soon as it is recycled, so, in a sense, this has the same effect as turning off the weak roots flag immediately after class unloading. 

 Also, the weak roots phase itself cannot have regions recycled because it relies on accurate mark information (recycling clears live data and resets the TAMS). We _could_ work around this by preserving the mark data (perhaps decoupling TAMS reset from region recycling). But changing the `gc_state` currently requires either a safepoint or a handshake (while holding the `Thread_lock`). I haven't thought all the way through this, but something like this (psuedo-code) might be possible:

```C++
vmop_entry_final_mark();

// Complete class unloading, since it actually _needs_ the oops (still need to forbid trash recycling here).
entry_class_unloading();

// Recycle trash, but do not reset TAMS (weak roots needs TAMS to decide reachability of referents).
entry_cleanup_early();

// Complete weak roots. There are no more trash regions and we don't have to change gc_state
entry_weak_refs();
entry_weak_roots();

What do you think? This would be a separate PR of course, but do you see any reason something like this wouldn't work? I'd expect some asserts to break if we allocate into a new region with TAMS > bottom.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23951#discussion_r1989959925