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

[Viktor Klang]

On Wed, 8 Nov 2023 17:24:45 GMT, Rémi Forax <forax at openjdk.org> wrote:

>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 530:
> 
>> 528:          * @param <R> the type of results this integrator can produce
>> 529:          */
>> 530:         @ForceInline
> 
> If we add this kind of the methods, we should add them on all function interfaces of java.util.function and java.util.stream.

Seems out of scope for this JEP

> src/java.base/share/classes/java/util/stream/GathererOp.java line 50:
> 
>> 48:  */
>> 49: final class GathererOp<T, A, R> extends ReferencePipeline<T, R> {
>> 50:     @SuppressWarnings("unchecked")
> 
> Could you explain why you need a SuppressWarnings here, especially the cast to (ReferencePipeline<?,T>) in the else case

Suggested improvements are most welcome! 👍

> src/java.base/share/classes/java/util/stream/GathererOp.java line 95:
> 
>> 93:         public void accept(X x) {
>> 94:             final var b = rightMost;
>> 95:             (b == null ? (rightMost = new NodeBuilder.Builder<>()) : b).accept(x);
> 
> I think this code is unecessarily hard to read.
> 
> var rightMost = this.rightMost;
> if (rightMost == null) {
>   rightMost = this.rightMost = new NodeBuilder.Builder<>();
> }
> rightMost.accept(x);

Benchmark your change first. If it is cost-neutral I'm all for changing!

> src/java.base/share/classes/java/util/stream/GathererOp.java line 180:
> 
>> 178:                 finisher.accept(state, this);
>> 179:             sink.end();
>> 180:             state = null; // GC assistance
> 
> Not sure it really helps the GC, given that it may go through a GC barrier

When it helps, it helps, and when it doesn't it has negligible cost.

> src/java.base/share/classes/java/util/stream/GathererOp.java line 232:
> 
>> 230:      */
>> 231:     @SuppressWarnings("unchecked")
>> 232:     private GathererOp(Gatherer<T, A, R> gatherer, GathererOp<?, ?, T> upstream) {
> 
> Is it not better to use a static method named by example `fuse`, i don't understand why it has to be a constructor.

It is only ever exposed via a static method.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386998422
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386999183
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387000558
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387002929
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387003542