On Wed, 17 Jan 2024 21:16:02 GMT, Joshua Cao <d...@openjdk.org> wrote:
>> ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` ->
>> `transfer()`. When coming from the copy constructor, the Map is empty, so
>> there is nothing to transfer. But `transfer()` will still copy all the empty
>> nodes from the old table to the new table.
>>
>> This patch avoids this work by only calling `tryPresize()` if the table is
>> already initialized. If `table` is null, the initialization is deferred to
>> `putVal()`, which calls `initTable()`.
>>
>> ---
>>
>> ### JMH results for testCopyConstructor
>>
>> before patch:
>>
>>
>> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
>> 937395.686 ±(99.9%) 99074.324 ns/op [Average]
>> (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184
>> CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution)
>>
>>
>> after patch:
>>
>>
>> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
>> 620871.469 ±(99.9%) 59195.406 ns/op [Average]
>> (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419
>> CI (99.9%): [561676.063, 680066.875] (assumes normal distribution)
>>
>>
>> Average time is decreased by about 33%.
>>
>> ### JMH results for testPutAll (size = 10000)
>>
>> before patch:
>>
>>
>> Result
>> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>> 4315291.542 ±(99.9%) 336034.190 ns/op [Average]
>> (min, avg, max) = (3974688.194, 4315291.542, 4871772.209), stdev =
>> 314326.589
>> CI (99.9%): [3979257.352, 4651325.731] (assumes normal distribution)
>>
>>
>> after patch:
>>
>>
>> Result
>> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>> 3006955.723 ±(99.9%) 271757.969 ns/op [Average]
>> (min, avg, max) = (2801264.198, 3006955.723, 3553084.135), stdev =
>> 254202.573
>> CI (99.9%): [2735197.754, 3278713.692] (assumes normal distribution)
>>
>>
>> Average time is decreased about 30%.
>
> Joshua Cao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> putAll presize based on sum on both map sizes
Thanks for the latest changes. The patch looks good now, except that I think
the benchmark could be made more accurate (see inline comments). This will
probably result in an even higher improvement for `putAll()`.
test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:
> 120: @Benchmark
> 121: public ConcurrentHashMap<Integer, Integer>
> testConcurrentHashMapPutAll() {
> 122: ConcurrentHashMap<Integer, Integer> map = new
> ConcurrentHashMap<>();
I think this benchmark could be made more accurate by creating the new,
temporary map with the right initial size (i.e. `ConcurrentHashMap<>(nkeys)`)
to avoid calls to `tryPresize()` in this setup step.
-------------
Changes requested by simonis (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1837027167
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1462205109
