RFR: 8345423: Shenandoah: Parallelize concurrent cleanup [v6]

[William Kemper]

On Wed, 4 Dec 2024 21:50:26 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 592:
>> 
>>> 590:   shenandoah_assert_heaplocked();
>>> 591:   if (is_trash() && _recycling.try_set()) {
>>> 592:     if (is_trash()) {
>> 
>> Is it necessary to check `is_trash` a second time while the heap lock is held? Also, if it _is_ necessary, then it seems like we should `_recycling.unset` in the scope where `_recycling.try_set` happened. As it is, if the second check for `is_trash` was `false`, then the code would not `_recycling.unset`.
>
> This method is only called by mutators with heap lock, is_trash is not tested before calling the method, it is worth to test before calling _recycling.try_set(), otherwise mutator will mostly(fast path) behaves like:
> 1. _recycling.try_set() -> true (always try to perform CAS, it is slower, we want to void it in fast path).
> 2. is_trash() -> false and skip the recycling. 
> 3. _recycling.unset()  (Should be also avoided in fast path)
> 
> But we want to fast path for mutator to be like: `is_trash() -> false && _recycling.is_set() -> false`.
> 
> 
> I have removed `is_trash` test from the code path executed by concurrent cleanup in the new version, that one is not needed since `is_trash` is tested in `ShenandoahRecycleTrashedRegionsTask`

Okay, I get it. The second test on line 593 is necessary because the gc workers don't hold the lock and could _in theory_ recycle the region between the first `is_trash` check on 592 and the `_recycling.try_set`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22538#discussion_r1871889122