RFR(s): 8060192: Add default method Collection.toArray(generator)

[Remi Forax]

----- Mail original -----
> De: "Stuart Marks" <stuart.marks at oracle.com>
> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Vendredi 15 Juin 2018 03:02:04
> Objet: RFR(s): 8060192: Add default method Collection.toArray(generator)

> Hi all,

Hi Stuart,

> 
> I wanted to restart review of the changeset for bug JDK-8060192 [1]. The latest
> webrev is here: [2].
> 
> The previous review was from December 2017: [3]. The only difference between the
> current changeset and the previous one is the removal of the overriding
> implementations (see explanation below). Much of the discussion in the previous
> review thread was around performance and the shape of the API.
> 
> # Performance
> 
> I previously had done some benchmarking on this and reported the results: [4].
> (I've recently re-done the benchmarks and conclusions are essentially the same.)
> 
> The upshot is that implementations that use Arrays.copyOf() are the fastest,
> probably because it's an intrinsic. It can avoid zero-filling the freshly
> allocated array because it knows the entire array contents will be overwritten.
> This is true regardless of what the public API looks like. The default
> implementation calls generator.apply(0) to get a zero-length array and then
> simply calls toArray(T[]). This goes through the Arrays.copyOf() fast path, so
> it's essentially the same speed as toArray(new T[0]).
> 
> The overrides I had previously provided in specific implementation classes like
> ArrayList actually are slower, because the allocation of the array is done
> separately from filling it. This necessitates the extra zero-filling step. Thus,
> I've removed the overrides.


for ArrayList, you may use a code like this,
  <T> T[] toArray(IntFunction<T[]> generator) {
    return (T[]) Arrays.copyOf(elementData, size, generator.apply(0).getClass());
  }
so you win only one comparison (yeah !), which can be perfectly predicted, so you should not see any perf difference :)

> 
> # API Shape
> 
> There was some discussion about whether it would be preferable to have a Class
> parameter as a type token for the array component type or the array type itself,
> instead of using an IntFunction generator. Most of this boils down to what
> people are comfortable with. However, there are a few points that make the
> generator function approach preferable.
> 
> One point in favor of using a generator is that Stream already has a similar
> toArray(generator) method.
> 
> Comparing this to the type token alternatives, for simple tasks like converting
> Collection<String> to String[], things are about equal:
> 
>     toArray(String[]::new)
>     toArray(String.class)
>     toArray(String[].class)
> 
> However, things are hairier if the element type of the collection is generic,
> such as Collection<List<String>>. The result type is a generic array
> List<String>[]. Using class literals or array constructor references will
> necessitate using an unchecked cast, because none of these can be used to
> represent a generic type.
> 
> However, it's possible to use a method reference to provide a properly generic
> IntFunction. This would enable the toArray(generator) method to be used without
> any unchecked casts. (The method it refers to might need have an unchecked cast
> within it, though.) Such a method could also be reused for the corresponding
> Stream.toArray(generator) method.

List<String>.class or List<String>[].class do not work either.

> 
> For these reasons I'd like to proceed with adding toArray(generator) API.
> 

so thumb up for me !

> Thanks,
> 
> s'marks

Rémi

> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8060192
> 
> [2] http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.3/
> 
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050325.html
> 
> [4]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050585.html