On Mon, 3 Jun 2024 16:47:20 GMT, jengebr <[email protected]> wrote:
> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless
> cloning of Object[0] instances. This cloning is intended to prevent callers
> from changing array contents, but many `CopyOnWriteArrayList`s are allocated
> to size zero, or are otherwise maintained empty, so cloning is unnecessary.
>
> Results from the included JMH benchmark:
> Before:
>
> Benchmark Mode Cnt
> Score Error Units
> CopyOnWriteArrayListBenchmark.clear avgt 5
> 74.487 ± 1.793 ns/op
> CopyOnWriteArrayListBenchmark.clearEmpty avgt 5
> 27.918 ± 0.759 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArray avgt 5
> 16.656 ± 0.375 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt 5
> 15.415 ± 0.489 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt 5
> 21.608 ± 0.363 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt 5
> 15.374 ± 0.260 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt 5
> 15.688 ± 0.350 ns/op
>
>
> After:
>
> Benchmark Mode Cnt
> Score Error Units
> CopyOnWriteArrayListBenchmark.clear avgt 5
> 75.365 ± 2.092 ns/op
> CopyOnWriteArrayListBenchmark.clearEmpty avgt 5
> 20.803 ± 0.539 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArray avgt 5
> 16.808 ± 0.582 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt 5
> 12.980 ± 0.418 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt 5
> 21.627 ± 0.173 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt 5
> 12.864 ± 0.408 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt 5
> 12.931 ± 0.255 ns/op
Some initial comments. @DougLea might want to give it a sanity check.
Note there is a jcheck failure due to whitespace issues. Plus, I think the PR
should still be named "8332842: Optimize empty CopyOnWriteArrayList
allocations"?
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java line
338:
> 336: */
> 337: public Object[] toArray() {
> 338: return getArray().length == 0 ? getArray() : getArray().clone();
Unfortunately, I think this is against the spec for this method, which
explicitly says the method must allocate a new array. Yes, this change would be
within the spirit of the spec ("You can modify the array", which you cannot if
the object is empty), but is against the letter of it.
-------------
PR Review: https://git.openjdk.org/jdk/pull/19527#pullrequestreview-2094435376
PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2145853813
PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1624791561
