RFR: 8351444: Shenandoah: Class Unloading may encounter recycled oops

[Aleksey Shipilev]

On Fri, 7 Mar 2025 21:47:31 GMT, William Kemper <wkemper at openjdk.org> wrote:

> Unloading classes may require a walk of unreachable oops. For this reason, it is not safe to recycle memory before class unloading is complete. This complements existing code to prevent mutators from recycling trash regions while weak roots is in progress.

This looks fine as the first step to this sequencing problem.

I do think we still have a conceptual problem of accessing the the oops in _trash_ regions. This patch blocks `trash` -> `empty` transition by delaying cleanup. This likely works well in release builds. I would expect debug builds to still complain we are touching the oop in `trash` region.

At class unloading, we can only have trash regions from the immediate trashing during region selection. So, in addition to this, I think we really need to move immediate trashing somewhere after class unloading as well. This would likely require more fiddling with heurstics: we do immediate trash there to see if we can take a shortcut cycle.

Changes requested by shade (Reviewer).

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 124:

> 122:   }
> 123: 
> 124:   // Allow resurrection of unreachable objects that are visited during concurrent class-unloading.

Let's not call it "Allow resurrection", which somewhat implies the object has full privileges to exist, i.e. can be inserted into the object graph back. But it really can't. We only do this because we were asked with `AS_NO_KEEPALIVE`.

Something like: "Allow runtime to see unreachable objects that are visited during concurrent class unloading"

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 153:

> 151:   }
> 152: 
> 153:   assert(heap->is_concurrent_weak_root_in_progress(), "Must be doing weak roots now");

This does not ring true when final mark is cancelled, or am I missing something?

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 163:

> 161:   // We cannot recycle regions because weak roots need to know what is marked in trashed regions.
> 162:   entry_weak_refs();
> 163:   entry_weak_roots();

Same as above: should probably be still protected by GC state checks to cover cancelled cases.

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23951#pullrequestreview-2670666176
PR Review: https://git.openjdk.org/jdk/pull/23951#pullrequestreview-2670700570
PR Review Comment: https://git.openjdk.org/jdk/pull/23951#discussion_r1987125159
PR Review Comment: https://git.openjdk.org/jdk/pull/23951#discussion_r1987139867
PR Review Comment: https://git.openjdk.org/jdk/pull/23951#discussion_r1987143228