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

[Peter Levart]

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

>> 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.

On a second thought, using `java.util.ImmutableCollections#listFromTrustedArrayNullsAllowed` is not a good idea, since the method expects the passed-in array to be trusted and use of this method in `Collections.addAll(col, array)` would wrap an untrusted array. Surely the resulting List would only be used as an argument to `col.addAll(list)`, but since neither `col` is trusted, it could "steal" the passed-in `list` and use it...
Some other implementation of immutable list array wrapper would be needed here.

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

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