Hi all,
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.
# 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.
For these reasons I'd like to proceed with adding toArray(generator) API.
Thanks,
s'marks
[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
