RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview) [v2]

[Viktor Klang]

On Thu, 9 Nov 2023 08:15:02 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> Viktor Klang has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Addressing review feedback
>>  - Make Gatherer.andThen take a wildcard for the rhs Gatherer state type
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 261:
> 
>> 259:     /**
>> 260:      * Returns an initializer which is the default initializer of a Gatherer.
>> 261:      * The returned initializer identifies that the owner Gatherer is stateless.
> 
> I know the text is logically correct but is there a way to be more intuitively correct? I mean, there are other initializers that could also mean the Gather is stateless.

@minborg Not really, from an evaluation-perspective only this specific initializer indicates true statelessness (other instances will signal that it is stateful even if it happens to use `null` as its state). True statelessness means that the initializer doesn't need to be invoked, and that any state does not need to be tracked.

> src/java.base/share/classes/java/util/stream/Gatherer.java line 267:
> 
>> 265:      * @param <A> the type of the state of the returned initializer
>> 266:      */
>> 267:     static <A> Supplier<A> defaultInitializer() {
> 
> Would code sharing imply performance implications due to profile pollution?

I guess that depends how deep from the callsite the profiler keeps information and how much of it gets inlined.

> src/java.base/share/classes/java/util/stream/Gatherer.java line 419:
> 
>> 417:      * @return the new {@code Gatherer}
>> 418:      */
>> 419:     static <T, R> Gatherer<T, Void, R> of(
> 
> Nit: In `Collector` the parameters are formatted in a different way. Should we be consistent with that existing formatting?

Which parameters? The type parameters or the method parameters?

> src/java.base/share/classes/java/util/stream/Gatherer.java line 535:
> 
>> 533:          */
>> 534:         @ForceInline
>> 535:         static <A, T, R> Integrator<A, T, R> of(Integrator<A, T, R> integrator) {
> 
> While this idiom is very convenient, there are some trap doors in combinations with method references and compatibility which is why we backed out of https://github.com/openjdk/jdk/pull/16213
> 
> It is good that the text mentions "lambda" but maybe we should explicitly mention that method references might introduce said problems?

This idiom is safer than casts since the casts are blind and need unchecked-annotations. I'm willing to take this approach for an evaluation during the Preview phase.

> src/java.base/share/classes/java/util/stream/Gatherers.java line 68:
> 
>> 66:      */
>> 67:     @SuppressWarnings("rawtypes")
>> 68:     enum Value implements Supplier, BinaryOperator, BiConsumer {
> 
> Can we move down this class at the end of Gatherers as it is an implementation concern? Or even better. move it to jdk.internal

What specifically are you looking to achieve?

> src/java.base/share/classes/java/util/stream/Gatherers.java line 326:
> 
>> 324: 
>> 325:     /**
>> 326:      * Gathers elements into fixed-size windows. The last window may contain
> 
> Returns a ...

You mean "Returns a Gatherer which gathers elements into ..." ?

> src/java.base/share/classes/java/util/stream/Gatherers.java line 347:
> 
>> 345:      * @throws IllegalArgumentException when windowSize is less than 1
>> 346:      */
>> 347:     public static <TR> Gatherer<TR, ?, List<TR>> windowFixed(int windowSize) {
> 
> In my opinion, it would be nicer to let `Gatherers` be a shopping window for cool gatherers. Ideally, I think only the docs, methods, parameters, and invariant assertions should be visible here. The rest could be tucked away under the covers.

@minborg While I think that would be fantastic from a theoretical standpoint -- however, as we've already seen with windowSliding and windowFixed, the need for performance and correctness will mean that the Gatherer implementations herein will likely not be "short and nice" which would be ideal from a "shopping window" perspective. And remember, it is not always the case that the cheese in the window is real. :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387726780
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387728774
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387730338
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387733407
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387734060
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387737838
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387736978