Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Bernd Eckenfels]

Hello,

What is the advantage of having such a narrow hashcode value space compared to 
the built in idendity hashcode? Would stocking to the object idendity not only 
reduce the footprint, but also make hash lookups faster? Without the unclear 
relationship to the op code?

Gruss
Bernd


--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von Joe 
Wang 
Gesendet: Friday, June 26, 2020 1:53:08 AM
An: Core-Libs-Dev 
Betreff: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

Hi,

Please review a fix to a BCEL regression. At issue was the addition of
hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
method was implemented to return the instruction's opcode that
unfortunately is mutable. The problem hasn't showed up until the code
path led to calls to remove from a HashSet a target that has been
changed since it was added to the HashSet. The proposed patch is to make
the hashCode final/immutable.

This patch implies that a target object is considered the same one even
if its field values may have been changed. It therefore may not be
appropriate in other situations (or may cause problems). However, since
it had always had no hashCode() override before BCEL 6.0, thereby
relying on Object's identity hash code, its use case has been limited
and time-tested. It can benefit from this patch in that it provides the
same function as Object's hash code, and then serves as a reminder (to
any who might read the code) how it was used (objects considered to be
the same over the course as far as the hashCode is concerned).

JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe

RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

Hi,

Please review a fix to a BCEL regression. At issue was the addition of 
hashCode() method to Instruction.java in BCEL 6.0. This hashCode() 
method was implemented to return the instruction's opcode that 
unfortunately is mutable. The problem hasn't showed up until the code 
path led to calls to remove from a HashSet a target that has been 
changed since it was added to the HashSet. The proposed patch is to make 
the hashCode final/immutable.


This patch implies that a target object is considered the same one even 
if its field values may have been changed. It therefore may not be 
appropriate in other situations (or may cause problems). However, since 
it had always had no hashCode() override before BCEL 6.0, thereby 
relying on Object's identity hash code, its use case has been limited 
and time-tested. It can benefit from this patch in that it provides the 
same function as Object's hash code, and then serves as a reminder (to 
any who might read the code) how it was used (objects considered to be 
the same over the course as far as the hashCode is concerned).


JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/

Thanks,
Joe

Re: RFR(T): 8248358: ProblemList sun/nio/ch/TestMaxCachedBufferSize.java on macOSX

[Daniel D. Daugherty]

Thanks for the fast review Igor!

Dan

On 6/25/20 6:38 PM, Igor Ignatyev wrote:

Hi Dan,

LGTM

Thanks,
-- Igor



 JDK-8248358 ProblemList 
serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on 
Windows
 https://bugs.openjdk.java.net/browse/JDK-8248358

I guess you meant JDK-8248358 ProblemList 
sun/nio/ch/TestMaxCachedBufferSize.java on macOSX



On Jun 25, 2020, at 2:45 PM, Daniel D. Daugherty  
wrote:

Greetings,

I'm doing another round of reduce-the-noise in the CI in preparation
for the upcoming weekend... So I have another trivial review...

Here's the bug for the failures:

 JDK-8212812 sun/nio/ch/TestMaxCachedBufferSize.java timeout
 https://bugs.openjdk.java.net/browse/JDK-8212812

and here's the bug for the ProblemListing:

 JDK-8248358 ProblemList 
serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on 
Windows
 https://bugs.openjdk.java.net/browse/JDK-8248358

Here's the context diff:

$ hg diff
diff -r cf65909b98c5 test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txtThu Jun 25 15:00:59 2020 -0400
+++ b/test/jdk/ProblemList.txtThu Jun 25 17:39:01 2020 -0400
@@ -630,6 +630,8 @@

  java/nio/channels/Selector/Wakeup.java 6963118 windows-all

+sun/nio/ch/TestMaxCachedBufferSize.java 8212812 macosx-all
+
  


Thanks, in advance, for any comments, questions or suggestions.

Dan

Re: RFR(T): 8248358: ProblemList sun/nio/ch/TestMaxCachedBufferSize.java on macOSX

[Igor Ignatyev]

Hi Dan,

LGTM

Thanks,
-- Igor


> JDK-8248358 ProblemList 
> serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on 
> Windows
> https://bugs.openjdk.java.net/browse/JDK-8248358
I guess you meant JDK-8248358 ProblemList 
sun/nio/ch/TestMaxCachedBufferSize.java on macOSX


> On Jun 25, 2020, at 2:45 PM, Daniel D. Daugherty 
>  wrote:
> 
> Greetings,
> 
> I'm doing another round of reduce-the-noise in the CI in preparation
> for the upcoming weekend... So I have another trivial review...
> 
> Here's the bug for the failures:
> 
> JDK-8212812 sun/nio/ch/TestMaxCachedBufferSize.java timeout
> https://bugs.openjdk.java.net/browse/JDK-8212812
> 
> and here's the bug for the ProblemListing:
> 
> JDK-8248358 ProblemList 
> serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on 
> Windows
> https://bugs.openjdk.java.net/browse/JDK-8248358
> 
> Here's the context diff:
> 
> $ hg diff
> diff -r cf65909b98c5 test/jdk/ProblemList.txt
> --- a/test/jdk/ProblemList.txtThu Jun 25 15:00:59 2020 -0400
> +++ b/test/jdk/ProblemList.txtThu Jun 25 17:39:01 2020 -0400
> @@ -630,6 +630,8 @@
> 
>  java/nio/channels/Selector/Wakeup.java 6963118 windows-all
> 
> +sun/nio/ch/TestMaxCachedBufferSize.java 8212812 macosx-all
> +
>  
> 
> 
> Thanks, in advance, for any comments, questions or suggestions.
> 
> Dan
> 
> 
> 
> 
> 
> 

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

[Remi Forax]

- Mail original -
> De: "Remi Forax" 
> À: "Daniel Fuchs" 
> Cc: "Patrick Concannon" , "core-libs-dev" 
> 
> Envoyé: Jeudi 25 Juin 2020 23:46:08
> Objet: Re: RFR[8238286]: 'Add new flatMap stream operation that is more 
> amenable to pushing’

> - Mail original -
>> De: "Daniel Fuchs" 
>> À: "Remi Forax" , "Patrick Concannon"
>> 
>> Cc: "core-libs-dev" 
>> Envoyé: Jeudi 25 Juin 2020 11:28:00
>> Objet: Re: RFR[8238286]: 'Add new flatMap stream operation that is more 
>> amenable
>> to pushing’
> 
>> Hi Rémi,
> 
> Hi Daniel,
> 
>> 
>> On 25/06/2020 00:32, Remi Forax wrote:
>>> I get that you want to keep Consumer instead of Consumer 
>>> because
>>> Consumer is not a valid target type for a lambda, but the 
>>> BiConsumer
>>> should be able to take a ? super Consumer instead of just Consumer.
>> 
>> Though I don't dispute that a strict application of the rules of
>> covariance and contravariance would require to design a signature
>> that accepts a `? super Consumer` - how would you implement a
>> BiConsumer with a signature that doesn't take a Consumer?
>> 
>> Such an implementation would be unable to push anything downstream
>> without having to cast back the consumer to Consumer.
> 
> if i have already have a BiConsumer, Object>, i would like to
> be able to call Stream.mapMulti() with that bi-consumer as argument.

and obviously, i got it wrong, Consumer is not a super-type of 
Consumer, it should be a BiConsumer, Object> or a 
BiConsumer, Object>, etc.

> 
>> 
>> My personal preference would be to vote in favor of the simpler
>> signature - which IMO is more readable and easier to understand.
>> 
>> best regards,

regards,
Rémi

> 
>> 
> > -- daniel

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

[forax]

- Mail original -
> De: "Daniel Fuchs" 
> À: "Remi Forax" , "Patrick Concannon" 
> 
> Cc: "core-libs-dev" 
> Envoyé: Jeudi 25 Juin 2020 11:28:00
> Objet: Re: RFR[8238286]: 'Add new flatMap stream operation that is more 
> amenable to pushing’

> Hi Rémi,

Hi Daniel,

> 
> On 25/06/2020 00:32, Remi Forax wrote:
>> I get that you want to keep Consumer instead of Consumer 
>> because
>> Consumer is not a valid target type for a lambda, but the 
>> BiConsumer
>> should be able to take a ? super Consumer instead of just Consumer.
> 
> Though I don't dispute that a strict application of the rules of
> covariance and contravariance would require to design a signature
> that accepts a `? super Consumer` - how would you implement a
> BiConsumer with a signature that doesn't take a Consumer?
> 
> Such an implementation would be unable to push anything downstream
> without having to cast back the consumer to Consumer.

if i have already have a BiConsumer, Object>, i would like to 
be able to call Stream.mapMulti() with that bi-consumer as argument.

> 
> My personal preference would be to vote in favor of the simpler
> signature - which IMO is more readable and easier to understand.
> 
> best regards,

regards,
Rémi

> 
> -- daniel

RFR(T): 8248358: ProblemList sun/nio/ch/TestMaxCachedBufferSize.java on macOSX

[Daniel D. Daugherty]

Greetings,

I'm doing another round of reduce-the-noise in the CI in preparation
for the upcoming weekend... So I have another trivial review...

Here's the bug for the failures:

    JDK-8212812 sun/nio/ch/TestMaxCachedBufferSize.java timeout
    https://bugs.openjdk.java.net/browse/JDK-8212812

and here's the bug for the ProblemListing:

    JDK-8248358 ProblemList 
serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java 
on Windows

    https://bugs.openjdk.java.net/browse/JDK-8248358

Here's the context diff:

$ hg diff
diff -r cf65909b98c5 test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt    Thu Jun 25 15:00:59 2020 -0400
+++ b/test/jdk/ProblemList.txt    Thu Jun 25 17:39:01 2020 -0400
@@ -630,6 +630,8 @@

 java/nio/channels/Selector/Wakeup.java 6963118 windows-all

+sun/nio/ch/TestMaxCachedBufferSize.java 8212812 macosx-all
+
 


Thanks, in advance, for any comments, questions or suggestions.

Dan

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

[forax]

- Mail original -
> De: "Brian Goetz" 
> À: "Remi Forax" , "Patrick Concannon" 
> 
> Cc: "core-libs-dev" 
> Envoyé: Jeudi 25 Juin 2020 22:04:27
> Objet: Re: RFR[8238286]: 'Add new flatMap stream operation that is more 
> amenable to pushing’

> On 6/24/2020 7:32 PM, Remi Forax wrote:
>> I don't really like the name "mapMulti", because flatMap can also be called
>> mapMulti.
> 
> Public service announcement: remember just how frustrating it is for
> Patrick to have put in all this work and get only comments like "I don't
> like the name."
> 
> If you think everything about it is fantastic, except maybe the name,
> then you can say so, and then maybe make a comment on the name -- or not.
> 
> If you have anything more substantial to say, please don't comment on
> bikeshed colors like naming before you've made your more substantial
> comments -- or before everyone else has as well. Because all that will
> happen is we'll have 100 replies-to-replies about the name, and poor
> Patrick will get no substantive comments.

you can also read after the  "i don't like the name", there is an explanation, 
it starts with "because ..."

> 
> (Personally, I think the name is fine.  It is reminiscent of LinQ's
> "SelectMany", which is their flatMap.)

yes, that's exactly my point, flatMap can be called mapMulti too and my fear is 
that people will use mapMulti were they should use flatMap and vice-versa, 
mapMulti is not a good enough name.

also you're not fair because i'm not the only one that find the name is not 
good and you have cherry picked only one sentence of my reply.

Rémi

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

[Brian Goetz]

On 6/24/2020 7:32 PM, Remi Forax wrote:

I don't really like the name "mapMulti", because flatMap can also be called 
mapMulti.


Public service announcement: remember just how frustrating it is for 
Patrick to have put in all this work and get only comments like "I don't 
like the name."


If you think everything about it is fantastic, except maybe the name, 
then you can say so, and then maybe make a comment on the name -- or not.


If you have anything more substantial to say, please don't comment on 
bikeshed colors like naming before you've made your more substantial 
comments -- or before everyone else has as well. Because all that will 
happen is we'll have 100 replies-to-replies about the name, and poor 
Patrick will get no substantive comments.


(Personally, I think the name is fine.  It is reminiscent of LinQ's 
"SelectMany", which is their flatMap.)

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

[Anthony Vanelverdinghe]

Hi

Given the signature of flatMap is:
 Stream flatMap​(FunctionR>> mapper)


I'd like to propose the following signature for the new method:
 Stream builderMap(BiConsumerStream.Builder> mapper)


This way both methods are named "...Map", and the name "builderMap" 
follows naturally from the argument's type.
If the given mapper invokes Stream.Builder::build, an 
IllegalStateException should be thrown.


Kind regards,
Anthony

On 25/06/2020 02:58, Paul Sandoz wrote:

Hi,

We traded CPS style for reusing existing functional interfaces. Originally the 
signature (my first choice) was as you indicate.

By chance it just so happens that the current signature is the same shape as 
that for the accumulating argument type of the three arg collect terminal 
operation:

Stream
 R collect(Supplier supplier,
   BiConsumer accumulator,
   BiConsumer combiner);

IntStream
 R collect(Supplier supplier,
   ObjIntConsumer accumulator,
   BiConsumer combiner);

Same for the accumulator of a Collector too.

However, I suspect you would argue these terminal accumulation cases are 
different from the intermediate case, as we are not accumulating but passing or 
accepting (loosely returning) zero or more elements that replace the input 
element.

It’s my hope that generic specialization will allow the primitive stream types 
to fade into the background, along with the primitive functional interfaces. In 
that respect the addition of three functional interfaces for use on the 
primitive stream types is not so terrible.


Regarding the name, you should have seen the first one :-) it was terrible.

Here’s my few brush strokes on the bike shed. I wonder what people think of mapAccept. The specification 
talks about accepting elements, because that is the operative method name on Consumer. So we can say 
"T is replaced with the elements accepted by the Consumer", or “ The Consumer 
accepts the elements that replace T"

Paul.




On Jun 24, 2020, at 1:01 PM, John Rose  wrote:

I like this new API point a lot; it allows flexible, local, temporary
control inversion in the context of one stream transform.

What’s the performance model?  It seems like the SpinedBuffer
implementation makes a worst-case assumption about the number
of pending values, that there will be many instead of zero or one.

But I guess the pipeline stuff already works in terms of pushes, so
the good news might be that this is really just a drill-down from the
user API into the kinds of operations (push-flavored) that go on
most of the time.

OK, so I like the function but I have a beef with its bike shed
color.  First of all, this is a control-inversion (CPS) pattern,
which is very powerful but also notoriously hard to read.
I think that in Java APIs, at least in Stream APIs, code is
easier to read if the logical data flow is from left to right.

(This is a language-specific observation.  Apart from varargs,
Java method APIs read favorably when extra control arguments
are added onto the end of the argument list.  Also, the convention
for generic functional interfaces is that the return value type
goes to the right, e.g., R in Function.)

So the BiConsumer is backwards, because the logical return
should be written, if not as a true return (which would appear
at the end of type parameter lists), at the end of the incoming
parameters (and in the last type parameter).

I also think “multi” is needlessly “learned” sounding.  A simple
spatial preposition would work well: mapThrough, mapAcross, etc.
I think I prefer mapAcross because the term “across” can be read
adverbially: “we are mapping T across to Consumer”.

So:

mapAcross(BiConsumer> mapper)
mapAcrossToInt(BiConsumer mapper)
mapAcross​(IntObjConsumer mapper)

This does require additional FI’s like IntObjConsumer, but
I think that is a negligible cost.  Making the control inversion
*readable* is the high order bit here, not minimizing the number
of trivial FIs.

(I almost hear David Bowman, in his space suit, saying, “My API…
It’s full of bikesheds!”  There’s a meme for that.)

— John

On Jun 24, 2020, at 3:57 AM, Patrick Concannon  
wrote:

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:

` Stream mapMulti(BiConsumer, ? 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.

issue: https://bugs.openjdk.java.net/browse/JDK-8238286 

csr: https://bugs.openjdk.java.net/browse/JDK-8248166 


webrev: 

Re: RFR [1]6 8248326 Add a minimal serialization test for local records

[Daniel Fuchs]

Hi Chris,

Looks good to me. It might also be beneficial to double check
that the hash codes of the two objects are equal.

best regards,

-- daniel

On 25/06/2020 16:01, Chris Hegarty wrote:

While working on some record serialization changes recently, I noticed that we 
don’t have any coverage for local records. Here is a simple extension to an 
existing records test.

diff -r 4e186efa6cbf test/jdk/java/io/Serializable/records/BasicRecordSer.java
--- a/test/jdk/java/io/Serializable/records/BasicRecordSer.java Thu Jun 25 
09:54:19 2020 +0100
+++ b/test/jdk/java/io/Serializable/records/BasicRecordSer.java Thu Jun 25 
15:40:13 2020 +0100
@@ -132,6 +132,20 @@
  assertEquals(objDeserialized, objToSerialize);
  }
  
+/** Tests serializing and deserializing of local records. */

+@Test
+public void testLocalRecord() throws Exception {
+out.println("\n---");
+record Point(int x, int y) implements Serializable { }
+record Rectangle(Point bottomLeft, Point topRight) implements 
Serializable { }
+var objToSerialize = new Rectangle(new Point(0, 1), new Point (5, 6));
+out.println("serializing : " + objToSerialize);
+var objDeserialized = serializeDeserialize(objToSerialize);
+out.println("deserialized: " + objDeserialized);
+assertEquals(objToSerialize, objDeserialized);
+assertEquals(objDeserialized, objToSerialize);
+}
+
  /** Tests back references of Serializable record objects in the stream. */
  @Test
  public void testSerializableBackRefs() throws Exception {

-Chris.

RFR [1]6 8248326 Add a minimal serialization test for local records

[Chris Hegarty]

While working on some record serialization changes recently, I noticed that we 
don’t have any coverage for local records. Here is a simple extension to an 
existing records test.

diff -r 4e186efa6cbf test/jdk/java/io/Serializable/records/BasicRecordSer.java
--- a/test/jdk/java/io/Serializable/records/BasicRecordSer.java Thu Jun 25 
09:54:19 2020 +0100
+++ b/test/jdk/java/io/Serializable/records/BasicRecordSer.java Thu Jun 25 
15:40:13 2020 +0100
@@ -132,6 +132,20 @@
 assertEquals(objDeserialized, objToSerialize);
 }
 
+/** Tests serializing and deserializing of local records. */
+@Test
+public void testLocalRecord() throws Exception {
+out.println("\n---");
+record Point(int x, int y) implements Serializable { }
+record Rectangle(Point bottomLeft, Point topRight) implements 
Serializable { }
+var objToSerialize = new Rectangle(new Point(0, 1), new Point (5, 6));
+out.println("serializing : " + objToSerialize);
+var objDeserialized = serializeDeserialize(objToSerialize);
+out.println("deserialized: " + objDeserialized);
+assertEquals(objToSerialize, objDeserialized);
+assertEquals(objDeserialized, objToSerialize);
+}
+
 /** Tests back references of Serializable record objects in the stream. */
 @Test
 public void testSerializableBackRefs() throws Exception {

-Chris.

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Daniel Fuchs]

Thanks for fixing that Rahul!

Looks good.

best regards,

-- daniel

On 25/06/2020 10:22, Rahul Yadav wrote:

Hello,

Deprecated tag in LogRecord getter and setter for threadID was missing 
"since" and so i have added it.


webrev : http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/

- rahul

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

[Daniel Fuchs]

Hi Rémi,

On 25/06/2020 00:32, Remi Forax wrote:

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


Though I don't dispute that a strict application of the rules of
covariance and contravariance would require to design a signature
that accepts a `? super Consumer` - how would you implement a
BiConsumer with a signature that doesn't take a Consumer?

Such an implementation would be unable to push anything downstream
without having to cast back the consumer to Consumer.

My personal preference would be to vote in favor of the simpler
signature - which IMO is more readable and easier to understand.

best regards,

-- daniel

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Rahul Yadav]

Hello,

Deprecated tag in LogRecord getter and setter for threadID was missing 
"since" and so i have added it.


webrev : http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/

- rahul

On 22/06/2020 19:38, Daniel Fuchs wrote:

Hi Rahul,

Looks good to me as well.

Reviewed.

best regards,

-- daniel

On 19/06/2020 10:00, Rahul Yadav wrote:

Thank you Alan, updated webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html


- rahul

On 19/06/2020 08:43, Alan Bateman wrote:

On 18/06/2020 23:37, Rahul Yadav wrote:

Hi Alan,

Thank you for the feedback.I have updated the webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html

This looks quite good.

The comment in shortShortID has "any positive long less than 
Integer.MAX_VALUE" but it's actually <= MAX_VALUE.


I don't think MIN_SEQUENTIAL_THREAD_ID is used so I assume it can be 
removed.


The @return for setLongThreadID has a description "Log Record" but 
this should "this LogRecord".


Can you update SerializeLogRecordTest with clear instructions on how 
to generate the stream? This will help future maintainers that may 
have to update this test.


-Alan

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

[Tagir Valeev]

Hello!

To me, it looks like it's possible to make the better default
implementation. It could be done even as a separate static method:

static  Stream ofPusher(Consumer> pusher) {
  return StreamSupport.stream(new Spliterator<>() {
private Spliterator delegate;

@Override
public boolean tryAdvance(Consumer action) {
  initDelegate();
  return delegate.tryAdvance(action);
}

private void initDelegate() {
  if (delegate == null) {
Stream.Builder builder = Stream.builder(); // or use
SpinedBuffer directly
pusher.accept(builder);
delegate = builder.build().spliterator();
  }
}

@Override
public void forEachRemaining(Consumer action) {
  if (delegate != null) {
delegate.forEachRemaining(action);
  } else {
pusher.accept(action);
  }
}

@Override
public Spliterator trySplit() {
  initDelegate();
  return delegate.trySplit();
}

@Override
public long estimateSize() {
  return Long.MAX_VALUE;
}

@Override
public int characteristics() {
  return 0;
}
  }, false);
}

In this case, we are buffering only if short-circuit operation or
splitting is requested. Otherwise, forEachRemaining will just delegate
to the pusher.
Now, the default implementation could be rewritten as

 Stream mapMulti(Stream stream, BiConsumer, ? super T> mapper) {
  Objects.requireNonNull(mapper);
  return stream.flatMap(e -> ofPusher(sink -> mapper.accept(sink, e)));
}

And now, I don't think it's necessary to specialize it at all.
Probably it's not necessary to introduce mapMulti at all as well, as
now it's a trivial delegate to ofPusher.

With best regards,
Tagir Valeev.

On Wed, Jun 24, 2020 at 5:58 PM Patrick Concannon
 wrote:
>
> 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:
>
> ` Stream mapMulti(BiConsumer, ? 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.
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8238286 
> 
> csr: https://bugs.openjdk.java.net/browse/JDK-8248166 
> 
>
> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/ 
> 
> specdiff: 
> http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
>   
> 
>
>
> Kind regards,
> Patrick & Julia