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

[Xiaolong Peng]

On Wed, 4 Dec 2024 18:42:42 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix naming issue
>
> 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 might be 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. 

But we want to fast path for mutator to be like: `is_trash() -> false && _recycling.is_set() -> false`.


I have remove the `is_trash` from the code path executed by concurrent cleanup in the new version, that one is not needed since `is_trash` is tested in `ShenandoahRecycleTrashedRegionsTask`

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

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