RFR: 8343704: Bad GC parallelism with processing Cleaner queues [v4]

[Kelvin Nilsen]

On Wed, 13 Nov 2024 15:38:35 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> See the bug for more discussion and reproducer. This PR replaces the ad-hoc linked list with the `ArrayList` wrapper that manages synchronization, search and replacements efficiently. Arrays are easy targets for GC. There are possible improvements here, most glaring is parallelism that is currently knee-capped by global synchronization. The synchronization scheme follows what we have in original code, and I think it is safer to continue with it right now.
>> 
>> I'll put performance data in a separate comment.
>> 
>> Additional testing:
>>  - [x] Original reproducer improves drastically
>>  - [x] New microbenchmark shows no regression on "churning" tests, which covers insertion/removal perf
>>  - [x] New microbenchmark shows improvement on Full GC times (crude, but repeatable), serves as a proxy for reproducer
>>  - [x] `java/lang/ref` tests in release 
>>  - [x] `all` tests in fastdebug
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8343704-cleaner-gc
>  - Merge branch 'master' into JDK-8343704-cleaner-gc
>  - Touchups: assert index, polish commits
>  - Fix

src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 290:

> 288:                 list.remove(lastIdx);
> 289: 
> 290:                 // Capacity control: trim the backing storage if it looks like

I think we need to set maxIdx to lastIdx - 1 here.  Otherwise, we'll never trim back the storage.  maxIdx can only increase in insert() as currently implemented.

src/java.base/share/classes/jdk/internal/ref/PhantomCleanable.java line 71:

> 69:         this.index = -1;
> 70:         this.list = CleanerImpl.getCleanerImpl(cleaner).activeList;
> 71:         list.insert(this);

For consistency, maybe use "this.list.insert(this)", or remove "this" from the preceding two lines.

test/micro/org/openjdk/bench/java/lang/ref/CleanerChurn.java line 56:

> 54:             }
> 55:         }
> 56:     }

Can we test whether lists get shrunk after lots of removes?  Maybe not practical without leaking implementation details through the API.  It looks to me that current implementation will not shrink.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1840866070
PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1840870321
PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1840879995