On Wed, 3 Jan 2024 14:34:50 GMT, Aleksey Shipilev <sh...@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%.
>
> src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
> 851:
>
>> 849: */
>> 850: public ConcurrentHashMap(Map<? extends K, ? extends V> m) {
>> 851: this(m.size(), LOAD_FACTOR, 1);
>
> This looks like just `this(m.size())`, right? Not sure if we want to save an
> additional chained constructor call, though.
Yep, same thing. I'll change it.
> test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:
>
>> 120: public void testCopyConstructor() {
>> 121: ConcurrentHashMap<Integer, Integer> clone = new
>> ConcurrentHashMap<>(staticMap);
>> 122: dummy = clone;
>
> Is this for preventing dead-code elimination? If so, just do:
>
>
> return new ConcurrentHashMap<>(staticMap);
I tried this. I don't think its working. When I just return the map, we lose
the improvements in performance, and the benchmark overall just runs much
faster. I'm guessing the map got DCE'ed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440798729
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440875149
