RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

Сергей Цыпанов sergei.tsypanov at yandex.ru
Fri Dec 18 14:17:39 UTC 2020 

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" <simon at 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+forax at openjdk.java.net> wrote:
>
>>  On Thu, 17 Dec 2020 10:15:23 GMT, Сергей Цыпанов <github.com+
>>  10835776+stsypanov at 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

More information about the core-libs-dev mailing list