Hi Simon, I've removed 'but this method is likely to run significantly faster under most implementations.' so the doc is cleaner now
Cheers, Sergey 17.12.2020, 16:38, "Simon Roberts" <si...@dancingcloudservices.com>: > When updating this, might you take the opportunity to remove the ambiguous > antecedent too? The use of "this method" when there are two methods in the > discussion (Collections.addAll, and the proximate one; Collection.addAll) > is unclear (indeed, one could argue the original text might have intended > to use "this method" to refer to Collection.addAll, in which interpretation > it's correct :) > > A more specific comment on the lines of "Collection.addAll is likely to run > faster..." would entirely avoid the ambiguity. > > Perhaps that's the intended solution anyway, in which case, I apologize for > the distraction. > > On Thu, Dec 17, 2020 at 3:38 AM Rémi Forax <github.com+ > 828220+fo...@openjdk.java.net> wrote: > >> On Thu, 17 Dec 2020 10:15:23 GMT, Сергей Цыпанов <github.com+ >> 10835776+stsypa...@openjdk.org> wrote: >> >> >> Hello, I feel like this was previously discussed in >> https://mail.openjdk.java.net/pipermail/core-libs-dev/ but since I cannot >> find original mail I post this here. >> >> >> >> Currently `Collections.addAll()` is implemented and documented as: >> >> /** >> >> * ... >> >> * The behavior of this convenience method is identical to that of >> >> * {@code c.addAll(Arrays.asList(elements))}, but this method is >> likely >> >> * to run significantly faster under most implementations. >> >> */ >> >> @SafeVarargs >> >> public static <T> boolean addAll(Collection<? super T> c, T... >> elements) { >> >> boolean result = false; >> >> for (T element : elements) >> >> result |= c.add(element); >> >> return result; >> >> } >> >> >> >> But it practice the notation `this method is likely to run >> significantly faster under most implementations` is completely wrong. When >> I take this [benchmark]( >> >> https://github.com/stsypanov/benchmarks/blob/master/benchmark-runners/src/main/java/com/luxoft/logeek/benchmark/collection/CollectionsAddAllVsAddAllBenchmark.java) >> and run it on JDK 14 I get the following results: >> >> (collection) >> (size) Score Error Units >> >> addAll ArrayList >> 10 37.9 ± 1.9 ns/op >> >> addAll ArrayList >> 100 83.8 ± 3.4 ns/op >> >> addAll ArrayList >> 1000 678.2 ± 23.0 ns/op >> >> collectionsAddAll ArrayList >> 10 50.9 ± 1.1 ns/op >> >> collectionsAddAll ArrayList >> 100 751.4 ± 47.4 ns/op >> >> collectionsAddAll ArrayList >> 1000 8839.8 ± 710.7 ns/op >> >> >> >> addAll HashSet >> 10 128.4 ± 5.9 ns/op >> >> addAll HashSet >> 100 1864.2 ± 102.4 ns/op >> >> addAll HashSet >> 1000 16615.5 ± 1202.6 ns/op >> >> collectionsAddAll HashSet >> 10 172.8 ± 6.0 ns/op >> >> collectionsAddAll HashSet >> 100 2355.8 ± 195.4 ns/op >> >> collectionsAddAll HashSet >> 1000 20364.7 ± 1164.0 ns/op >> >> >> >> addAll ArrayDeque >> 10 54.0 ± 0.4 ns/op >> >> addAll ArrayDeque >> 100 319.7 ± 2.5 ns/op >> >> addAll ArrayDeque >> 1000 3176.9 ± 22.2 ns/op >> >> collectionsAddAll ArrayDeque >> 10 66.5 ± 1.4 ns/op >> >> collectionsAddAll ArrayDeque >> 100 808.1 ± 55.9 ns/op >> >> collectionsAddAll ArrayDeque >> 1000 5639.6 ± 240.9 ns/op >> >> >> >> addAll CopyOnWriteArrayList >> 10 18.0 ± 0.7 ns/op >> >> addAll CopyOnWriteArrayList >> 100 39.4 ± 1.7 ns/op >> >> addAll CopyOnWriteArrayList >> 1000 371.1 ± 17.0 ns/op >> >> collectionsAddAll CopyOnWriteArrayList >> 10 251.9 ± 18.4 ns/op >> >> collectionsAddAll CopyOnWriteArrayList >> 100 3405.9 ± 304.8 ns/op >> >> collectionsAddAll CopyOnWriteArrayList >> 1000 247496.8 ± 23502.3 ns/op >> >> >> >> addAll ConcurrentLinkedDeque >> 10 81.4 ± 2.8 ns/op >> >> addAll ConcurrentLinkedDeque >> 100 609.1 ± 26.4 ns/op >> >> addAll ConcurrentLinkedDeque >> 1000 4494.5 ± 219.3 ns/op >> >> collectionsAddAll ConcurrentLinkedDeque >> 10 189.8 ± 2.5 ns/op >> >> collectionsAddAll ConcurrentLinkedDeque >> 100 1660.0 ± 62.0 ns/op >> >> collectionsAddAll ConcurrentLinkedDeque >> 1000 17649.2 ± 300.9 ns/op >> >> >> >> addAll:·gc.alloc.rate.norm ArrayList >> 10 160.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ArrayList >> 100 880.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ArrayList >> 1000 8080.3 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayList >> 10 80.0 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayList >> 100 1400.2 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayList >> 1000 15025.6 ± 0.1 B/op >> >> >> >> addAll:·gc.alloc.rate.norm HashSet >> 10 464.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm HashSet >> 100 5328.5 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm HashSet >> 1000 48516.7 ± 0.1 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm HashSet >> 10 464.0 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm HashSet >> 100 5328.5 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm HashSet >> 1000 48516.6 ± 0.1 B/op >> >> >> >> addAll:·gc.alloc.rate.norm ArrayDeque >> 10 112.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ArrayDeque >> 100 560.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ArrayDeque >> 1000 4160.5 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayDeque >> 10 112.0 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayDeque >> 100 1048.1 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ArrayDeque >> 1000 14929.4 ± 0.0 B/op >> >> >> >> addAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 10 88.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 100 448.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 1000 4048.1 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 10 456.0 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 100 22057.2 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm CopyOnWriteArrayList >> 1000 2020150.3 ± 7.3 B/op >> >> >> >> addAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 10 312.0 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 100 2472.1 ± 0.0 B/op >> >> addAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 1000 24073.7 ± 0.1 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 10 288.0 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 100 2448.3 ± 0.0 B/op >> >> collectionsAddAll:·gc.alloc.rate.norm ConcurrentLinkedDeque >> 1000 24051.4 ± 0.3 B/op >> >> There's never a case when `Collections.addAll` is fater - on the >> contrary `c.addAll(Arrays.asList())` always wins. Pay attention especially >> to dramatic difference for array-based collection. >> >> >> >> So I propose to reimplement the method by simply delegating to >> `Arrays.asList` because the spec declares identical behaviour and to remove >> perfomance notation from JavaDoc. >> > >> > Looks like I've found the original ticket: >> https://bugs.openjdk.java.net/browse/JDK-8193031 >> >> Apart from the @SuppressWarnings, this looks good to me. >> And i like the irony of this. >> >> ------------- >> >> PR: https://git.openjdk.java.net/jdk/pull/1764 > > -- > Simon Roberts > (303) 249 3613
