Hi Sergey,
There are some links to a few other discussion threads in the bug report [1].
I've added a link to this one. :-)
It's too late for JDK 13, which entered RDP1 today. However, the JDK 14 source
base is open, and we can proceed there.
In one of the previous discussions, I had suggested an alternative which is to
add a default method ("addEach") that adds to a collection all the elements of a
given array. This was probably a distraction from that discussion, and it might
have had the side effect of derailing that discussion. So, sorry about that.
Let's focus on Martin's proposal. Basically this replaces the for-loop with code
that wraps the array with Arrays.asList and then simply calls Collection.addAll.
There is also some spec cleanup.
Overall I think this is fine, but I did have a reservation. If we have
Collections.addAll(arraylist, array)
this would end up calling
arraylist.addAll(Arrays.asList(array))
The first thing ArrayList.addAll() does is
Object[] a = c.toArray();
which makes a copy of the input array. Right away, this seems like a waste.
On the other hand, I observed that doing two bulk array copies was actually
faster than making one copy using a for-loop. I don't remember what sizes I
checked. I'm a bit surprised, though, I would have thought that some sizes would
be faster and some slower. Perhaps someone should do some more benchmarks. But
that seems like an advantage of using addAll.
One thing I didn't think of previously was that using a for-loop to add elements
one-at-a-time might resize the destination ArrayList more than once, resulting
in some redundant copying, whereas a bulk copy with addAll() will resize the
destination at most once before doing the copy. That's another point in favor of
using addAll().
In summary, while this might seem like an obvious thing to do, it's not a
no-brainer, and there are some subtle tradeoffs. We might end up doing this
eventually, but it would be good to have a better handle on the tradeoffs and
potential impact.
Martin was working on this most recently. Martin, what do you think?
s'marks
On 6/13/19 4:39 AM, Сергей Цыпанов wrote:
Hello,
the issue [1] was opened almost two years ago,
but even today Collections.addAll still looks like this:
public static <T> boolean addAll(Collection<? super T> c, T... elements) {
boolean result = false;
for (T element : elements)
result |= c.add(element);
return result;
}
The most wide-spread collections in Java-world is ArrayList for which
Collections.addAll is likely to be slower then Collection.addAll(Arrays.asList).
The same is valid for ArrayDeque and especially for COWList. There's a webrev
[2] for mentioned ticket. The whole story begins in [3].
Could the change [2] be done for JDK 13?
Regards,
Sergey Tsypanov
1. https://bugs.openjdk.java.net/browse/JDK-8193031
2. http://cr.openjdk.java.net/~martin/webrevs/jdk/Collections-addAll
3.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050326.html
