On Fri, 15 Dec 2023 01:16:55 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%.
All right, good! I have comments about the benchmark:
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.
test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 83:
> 81: for (int i = 0; i < nkeys; ++i) {
> 82: staticMap.put(rng.next(), rng.next());
> 83: }
Can just merge this loop with the one above, reusing `rng.next()` for both key
and values for `staticMap`.
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);
-------------
PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1802240545
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440523638
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440517305
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440515260
