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

[Aleksey Shipilev]

On Fri, 22 Nov 2024 16:12:18 GMT, Viktor Klang <vklang at openjdk.org> wrote:

>> 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 17 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8343704-cleaner-gc
>>  - Improve CleanerGC benchmark
>>  - Check all elements are removable after random test
>>  - Use RandomFactory in test
>>  - Touchups
>>  - Merge branch 'master' into JDK-8343704-cleaner-gc
>>  - Drop --add-exports from the test
>>  - prev is not needed
>>  - Do not need -ea -esa in tests, our testing infra adds them already
>>  - Add the node cache
>>  - ... and 7 more: https://git.openjdk.org/jdk/compare/309ffe83...507401c7
>
> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 277:
> 
>> 275:          */
>> 276:         public synchronized void insert(PhantomCleanable<?> phc) {
>> 277:             if (head.size == NODE_CAPACITY) {
> 
> Given that `head.size` is a mutable member, it will be clearer to start out reading the current head.size into a local final variable and use that variable throughout the method where head.size is read.

I don't quite agree on this one: in `insert`, `head.size` is not stable, as we can switch the `head` in the middle of the method, when we insert another head. Keeping `head.size` in a local variable raises a question whether that local is out of sync. Given that the whole method is `synchronized`, I think the "stable point" is mostly everywhere in the method. The only place I can see this _might_ make sense is saving `head.size` on two adjacent lines after `head` is indeed stable. 

Overall, I think we are in nitpicking territory, and this is 15-th revision of this PR. I would prefer us not to do any changes that are not substantial.

> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 316:
> 
>> 314:             // as it is not the same element. This keeps all non-head
>> 315:             // nodes at full capacity.
>> 316:             if (head != phc.node || (phc.index != head.size - 1)) {
> 
> Creating a local final variable with the value of `head.size - 1` early in this method, and use that throughout (since `head.size - 1` is mentioned in multiple places (as indexing as well as assignment via head.size--) would make it a stable point in the logic.

I did this local for `head.size - 1` in one of my patches, but on balance thought it does not gain much clarity. I reinstated it now...

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1854354863
PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1854355690