RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll
Simon Roberts
simon at dancingcloudservices.com
Thu Dec 17 14:34:29 UTC 2020
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