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

[Peter Levart]

On Tue, 29 Dec 2020 10:21:07 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> "This message" referred to the entirety of that very comment of mine https://github.com/openjdk/jdk/pull/1764#issuecomment-748926986
>> 
>> I prepended that message with that clause (that is, the wording to the left of the colon) to make it clear that I didn't want to review or argue with anything said before that in that thread; it seems that clause made more harm than good.
>
> Hi,
> Sorry for joining late to this discussion, but I think that changing this method to delegate to c.addAll(Arrays.asList(...)) might not be the best thing to do here. Why?
> - This might look as a convenience method, but it takes only 2 characters less to type Collections.addAll(c, ...) than it takes to type c.addAll(Arrays.asList(...)), so we would basically end up with two ways of performing the same thing. I think we can make a method that would be worth more than that.
> - Pursuing performance is a noble goal, but performance is not everything. Predictability and guarantees are also important. The convenience method, being varargs method, can also be invoked with an array in place of varargs argument. Current implementation is trusted (part of JDK) and guarantees to never modify the passed-in array argument. If this was modified to just delegate to Collection.addAll(Arrays.asList(array)), it depends on the concrete implementation of the .addAll method of the implementation class of the collection. Misbehaving collection implementation could modify the passed-in Arrays.asList and such modifications would propagate to the array.
> 
> So I would preferably do one of two things here. Either change the javadoc to describe current behaviour better, or use some other immutable List wrapper different than Arrays.asList() which is mutable. WDYT?

Hint: you could use `java.util.ImmutableCollections#listFromTrustedArrayNullsAllowed` if only this method would allow other types of arrays, not just Object[]... I really don't know why this restriction couldn't be lifted as the captured array is fully encapsulated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1764