On Fri, 22 Nov 2024 16:12:18 GMT, Viktor Klang <vkl...@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
