Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Remi Forax]

Hi Patrick,

----- Mail original -----
> De: "Patrick Concannon" <patrick.concannon at oracle.com>
> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Mercredi 24 Juin 2020 12:57:01
> Objet: RFR[8238286]:  'Add new flatMap stream operation that is more amenable to pushing’

> Hi,

Hi,

> 
> Could someone please review myself and Julia's RFE and CSR for JDK-8238286 -
> 'Add new flatMap stream operation that is more amenable to pushing’?
> 
> This proposal is to add a new flatMap-like operation:
> 
> `<R> Stream<R> mapMulti(BiConsumer<Consumer<R>, ? super T> mapper)`
> 
> to the java.util.Stream class. This operation is more receptive to the pushing
> or yielding of values than the current implementation that internally assembles
> values (if any) into one or more streams. This addition includes the primitive
> variations of the operation i.e. mapMultiToInt, IntStream mapMulti, etc.

I don't really like the name "mapMulti", because flatMap can also be called mapMulti.
I'm sure someone will come with a better name, mapPush ?, mapYield ?

Why the BiConsumer takes a T as the second parameter and not as the first like in the bug description ?

I get that you want to keep Consumer<R> instead of Consumer<? super R> because Consumer<? super R> is not a valid target type for a lambda, but the BiConsumer should be able to take a ? super Consumer<R> instead of just Consumer<R>.

In Stream.java, in the javadoc both the examples of mapMulti are using wrapper types instead of primitives, which is not the way we want people to use streams, stream of wrapper types cause the perf issue.

In *Stream.java, in the default method, the 'this' in 'this'.flatMap is useless.
 
In *Pipeline.java, i don't think you need the lambda downstream::accept given that a Sink is already a Consumer,
instead of
  mapper.accept(downstream::accept, u);
using the downstream directly should work
  mapper.accept(downstream, u);

also it seems to me that the implementation of cancellationRequested() is useless given that Sink.Chained*::cancellationRequested already delegates to the downstream sink.
 
I've also noticed that in ReferencePipelien.java, in <R> Stream<R> mapMulti(BiConsumer<Consumer<R>, ? super P_OUT> mapper), the cache field is missing, but its should be necessary anymore.

> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8238286
> <https://bugs.openjdk.java.net/browse/JDK-8238286>
> csr: https://bugs.openjdk.java.net/browse/JDK-8248166
> <https://bugs.openjdk.java.net/browse/JDK-8248166>
> 
> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/
> <http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/>
> specdiff:
> http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
>   <http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html>
> 
> 
> Kind regards,
> Patrick & Julia

regards,
Rémi