Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

@AlanBateman I'm done with the changes you requested and kindly like to ask 
where to go from here.

Great ideas, will do as you say, stay tuned! Didn't know that using `var` is a 
no-go, sorry for that. Pattern matching is definitively a great idea!

Replaced `var` by actual type in 
https://github.com/openjdk/jdk/pull/4263/commits/b3c62b0951124550a686386b9419f7246c214dc6.

Replaced casting by pattern matching for `instanceof` in 
https://github.com/openjdk/jdk/pull/4263/commits/ac62cdb9acffc133bfaaa97ee77d5c7b3b704994.

There had been no more comments since one week. How to proceed?

As there had been no more change requests within one week, I assume this means 
you all rate this PR as read-to-review, so hereby I change its state and 
explicitly invite everybody to review this proposal. :-)

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On Sat, 5 Jun 2021 15:11:55 GMT, Markus KARG 
 wrote:

> @AlanBateman I'm done with the changes you requested and kindly like to ask 
> where to go from here.

Moving ChannelOutputStream to sun.nio.ch looks right. The implementation of 
transferTo will need a few rounds of cleanup, it's a look very messy right now. 
Most usages of var need to be replaced as it is impossible for the reader to 
know what the types are. Pattern matching for instanceof can be used to avoid 
the casts.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Tue, 8 Jun 2021 11:49:55 GMT, Alan Bateman  wrote:

>> @AlanBateman I'm done with the changes you requested and kindly like to ask 
>> where to go from here.
>
>> @AlanBateman I'm done with the changes you requested and kindly like to ask 
>> where to go from here.
> 
> Moving ChannelOutputStream to sun.nio.ch looks right. The implementation of 
> transferTo will need a few rounds of cleanup, it's a look very messy right 
> now. Most usages of var need to be replaced as it is impossible for the 
> reader to know what the types are. Pattern matching for instanceof can be 
> used to avoid the casts.

@AlanBateman Pushed the requested changes. More change requests? :-)

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Brian Burkhalter]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 113:

> 111: if ((off < 0) || (off > bs.length) || (len < 0) ||
> 112: ((off + len) > bs.length) || ((off + len) < 0)) {
> 113: throw new IndexOutOfBoundsException();

Could the bounds checking be done as follows?

`Objects.checkFromIndexSize(off, len, bs.length);`

Same comment applies to lines 146 and 219 in `java.nio.channels.Channels`.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Wed, 2 Jun 2021 09:03:03 GMT, Alan Bateman  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> src/java.base/share/classes/java/nio/channels/Channels.java line 145:
> 
>> 143:  * @since 18
>> 144:  */
>> 145: public static class ChannelOutputStream extends OutputStream {
> 
> This adds Channels.ChannelOutputStream to the API, you will need to justify 
> that.

You mean as a source comment or just here in this discussion thread?

In fact it might be better to not add it to a package with is part of the API, 
but to move it to the `sun` package, which is not, right?

The justification is that I need to refer to this class from 
`ChannelInputStream::transferTo()` to be able to get hold of this previously 
anonymous class's inner member "ch", which in turn is key to the whole story of 
this PR: Using NIO transfer between the channels instead of moving bytes 
through the JVM's memory.

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 148:
> 
>> 146: 
>> 147: @Override
>> 148: public long transferTo(final OutputStream out) throws IOException {
> 
> Please try to keep to existing style, e.g. most/all "final" are noise and can 
> be removed.

Sorry I am new do this project and didn't know that final is banned generally. 
To get it right: Is it banned only in parameters or also for variables and 
class members?

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 158:
> 
>> 156: for (long n = size - pos; i < n;
>> 157:   i += fc.transferTo(pos + i, Long.MAX_VALUE, oc));
>> 158: fc.position(size);
> 
> The patch is improving but needs cleanup so that it is easy to maintain. I 
> think I would move the I/O ops out of the update code and into the for 
> statement/block. Also this will need the update to the channel position to be 
> a finally block so that the it is adjusted when there are partial transfers.

Moved I/O operations into the `for` statement by 
https://github.com/openjdk/jdk/pull/4263/commits/562b1023e62c4ff0a36e55ebc119cee6fb69809c.

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 177:
> 
>> 175: }
>> 176: 
>> 177: final var bb = ByteBuffer.allocateDirect(TRANSFER_SIZE);
> 
> This will probably need to be changed to the use the TL buffer cache.

Uhm... Maybe this is a dumb beginner's question, but: What is "the TL buffer 
cache"?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Sat, 5 Jun 2021 07:25:44 GMT, Markus KARG 
 wrote:

>> You mean as a source comment or just here in this discussion thread?
>> 
>> In fact it might be better to not add it to a package with is part of the 
>> API, but to move it to the `sun` package, which is not, right?
>> 
>> The justification is that I need to refer to this class from 
>> `ChannelInputStream::transferTo()` to be able to get hold of this previously 
>> anonymous class's inner member "ch", which in turn is key to the whole story 
>> of this PR: Using NIO transfer between the channels instead of moving bytes 
>> through the JVM's memory.
>
> Will move the class (and the needed utility methods) to the `sun` package to 
> prevent addition to the API. Stay tuned. :-)

`ChannelOutputStream` isn't part of the API anymore as I moved it to the `sun` 
package by commit 
https://github.com/openjdk/jdk/pull/4263/commits/b6dc6a7eadb5168531459aa032f169b344cadb47.
 Thankyou for pointing this out!

>> Moved I/O operations into the `for` statement by 
>> https://github.com/openjdk/jdk/pull/4263/commits/562b1023e62c4ff0a36e55ebc119cee6fb69809c.
>
> Correctly positioning channel in case of exception by 
> https://github.com/openjdk/jdk/pull/4263/commits/ed49098a4712bf23cb6d218c27695717ba3312c2.

Honestly, for me the code looks perfectly comprehensible now, so I actually do 
not know what to simplify any further (besides adjusting to personal code style 
likes or dislikes). If there actually *is* something hard to read or 
understand, please don't hesitate to express your detailed concerns and/or 
propose an actual change. I will be happy to adopt it in this PR.

>> Uhm... Maybe this is a dumb beginner's question, but: What is "the TL buffer 
>> cache"?
>
> I think I found what you mean and will use it: 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/Util.java#L221

Done in 
https://github.com/openjdk/jdk/pull/4263/commits/8f589867154f8bc22ca6f6bb7a26d7ae841fe714.
 Thank you!

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Thu, 3 Jun 2021 17:29:14 GMT, Markus KARG 
 wrote:

>> src/java.base/share/classes/java/nio/channels/Channels.java line 145:
>> 
>>> 143:  * @since 18
>>> 144:  */
>>> 145: public static class ChannelOutputStream extends OutputStream {
>> 
>> This adds Channels.ChannelOutputStream to the API, you will need to justify 
>> that.
>
> You mean as a source comment or just here in this discussion thread?
> 
> In fact it might be better to not add it to a package with is part of the 
> API, but to move it to the `sun` package, which is not, right?
> 
> The justification is that I need to refer to this class from 
> `ChannelInputStream::transferTo()` to be able to get hold of this previously 
> anonymous class's inner member "ch", which in turn is key to the whole story 
> of this PR: Using NIO transfer between the channels instead of moving bytes 
> through the JVM's memory.

Will move the class (and the needed utility methods) to the `sun` package to 
prevent addition to the API. Stay tuned. :-)

>> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 148:
>> 
>>> 146: 
>>> 147: @Override
>>> 148: public long transferTo(final OutputStream out) throws IOException {
>> 
>> Please try to keep to existing style, e.g. most/all "final" are noise and 
>> can be removed.
>
> Sorry I am new do this project and didn't know that final is banned 
> generally. To get it right: Is it banned only in parameters or also for 
> variables and class members?

Checked the existing code and removed most `final`s but kept it only for 
immitable members and real constants in 
https://github.com/openjdk/jdk/pull/4263/commits/df1a57a12cc81bbdcb36d3caf66a7aea7cc542cb.

>> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 158:
>> 
>>> 156: for (long n = size - pos; i < n;
>>> 157:   i += fc.transferTo(pos + i, Long.MAX_VALUE, oc));
>>> 158: fc.position(size);
>> 
>> The patch is improving but needs cleanup so that it is easy to maintain. I 
>> think I would move the I/O ops out of the update code and into the for 
>> statement/block. Also this will need the update to the channel position to 
>> be a finally block so that the it is adjusted when there are partial 
>> transfers.
>
> Moved I/O operations into the `for` statement by 
> https://github.com/openjdk/jdk/pull/4263/commits/562b1023e62c4ff0a36e55ebc119cee6fb69809c.

Correctly positioning channel in case of exception by 
https://github.com/openjdk/jdk/pull/4263/commits/ed49098a4712bf23cb6d218c27695717ba3312c2.

>> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 177:
>> 
>>> 175: }
>>> 176: 
>>> 177: final var bb = 
>>> ByteBuffer.allocateDirect(TRANSFER_SIZE);
>> 
>> This will probably need to be changed to the use the TL buffer cache.
>
> Uhm... Maybe this is a dumb beginner's question, but: What is "the TL buffer 
> cache"?

I think I found what you mean and will use it: 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/Util.java#L221

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Brian Burkhalter]

On Tue, 8 Jun 2021 20:20:58 GMT, Markus KARG 
 wrote:

>> src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 113:
>> 
>>> 111: if ((off < 0) || (off > bs.length) || (len < 0) ||
>>> 112: ((off + len) > bs.length) || ((off + len) < 0)) {
>>> 113: throw new IndexOutOfBoundsException();
>> 
>> Could the bounds checking be done as follows?
>> 
>> `Objects.checkFromIndexSize(off, len, bs.length);`
>> 
>> Same comment applies to lines 146 and 219 in `java.nio.channels.Channels`.
>
> I'd like to abstain from changes in ChannelOutputStream, as I did not write 
> that code at all. It is simply moved from being an inner class. Please let's 
> concentrate on the code I actually wrote in this PR. Thanks.

That's fine. Sometimes we make small changes like that while we are in nearby 
code.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Tue, 8 Jun 2021 20:34:15 GMT, Brian Burkhalter  wrote:

>> I'd like to abstain from changes in ChannelOutputStream, as I did not write 
>> that code at all. It is simply moved from being an inner class. Please let's 
>> concentrate on the code I actually wrote in this PR. Thanks.
>
> That's fine. Sometimes we make small changes like that while we are in nearby 
> code.

I see. Changed by 
https://github.com/openjdk/jdk/pull/4263/commits/599181bd5289a1c31e0a3bb596c979a9e6491ccd.
 Thank you!

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Tue, 8 Jun 2021 19:26:14 GMT, Brian Burkhalter  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 113:
> 
>> 111: if ((off < 0) || (off > bs.length) || (len < 0) ||
>> 112: ((off + len) > bs.length) || ((off + len) < 0)) {
>> 113: throw new IndexOutOfBoundsException();
> 
> Could the bounds checking be done as follows?
> 
> `Objects.checkFromIndexSize(off, len, bs.length);`
> 
> Same comment applies to lines 146 and 219 in `java.nio.channels.Channels`.

I'd like to abstain from changes in ChannelOutputStream, as I did not write 
that code at all. It is simply moved from being an inner class. Please let's 
concentrate on the code I actually wrote in this PR. Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

src/java.base/share/classes/java/nio/channels/Channels.java line 145:

> 143:  * @since 18
> 144:  */
> 145: public static class ChannelOutputStream extends OutputStream {

This adds Channels.ChannelOutputStream to the API, you will need to justify 
that.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 148:

> 146: 
> 147: @Override
> 148: public long transferTo(final OutputStream out) throws IOException {

Please try to keep to existing style, e.g. most/all "final" are noise and can 
be removed.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 158:

> 156: for (long n = size - pos; i < n;
> 157:   i += fc.transferTo(pos + i, Long.MAX_VALUE, oc));
> 158: fc.position(size);

The patch is improving but needs cleanup so that it is easy to maintain. I 
think I would move the I/O ops out of the update code and into the for 
statement/block. Also this will need the update to the channel position to be a 
finally block so that the it is adjusted when there are partial transfers.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 177:

> 175: }
> 176: 
> 177: final var bb = ByteBuffer.allocateDirect(TRANSFER_SIZE);

This will probably need to be changed to the use the TL buffer cache.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Michael Bien]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 216:

> 214: }
> 215: 
> 216: return super.transferTo(out);

(i am no reviewer)
wouldn't this method be more intuitive if it wouldn't try to avoid using "else 
if" and "else"? If there are sequential if blocks with return in them, part of 
me is always trying to find the fall-through scenario, but in this case its all 
distinct code paths. Using branches would make that obvious + if there would be 
a fall through in future, the compiler would generate a "missing return" error 
right away. 

example:

if (out instanceof ChannelOutputStream cos) {

WritableByteChannel oc = cos.channel();
long i = 0L;

if (ch instanceof FileChannel fc) {

long pos = fc.position();
long size = fc.size();
try {
for (long n = size - pos; i < n;)
i += fc.transferTo(pos + i, Long.MAX_VALUE, oc);
return i;
} finally {
fc.position(pos + i);
}

} else if (oc instanceof FileChannel fc) {

long fcpos = fc.position();

if (ch instanceof SeekableByteChannel sbc) {

long pos = sbc.position();
long size = sbc.size();
try {
for (long n = size - pos; i < n;)
i += fc.transferFrom(ch, fcpos + i, Long.MAX_VALUE);
return i;
} finally {
fc.position(fcpos + i);
}

} else {

ByteBuffer bb = 
Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
try {
int r;
do {
i += fc.transferFrom(ch, fcpos + i, Long.MAX_VALUE);
r = ch.read(bb); // detect end-of-stream
if (r > -1) {
bb.flip();
while (bb.hasRemaining())
oc.write(bb);
bb.clear();
i += r;
}
} while (r > -1);
return i;
} finally {
fc.position(fcpos + i);
Util.releaseTemporaryDirectBuffer(bb);
}
}

} else {

ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
try {
for (int r = ch.read(bb); r > -1; r = ch.read(bb)) {
bb.flip();
while (bb.hasRemaining())
oc.write(bb);
bb.clear();
i += r;
}
return i;
} finally {
Util.releaseTemporaryDirectBuffer(bb);
}
}
} else {
return super.transferTo(out);
}

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Thu, 17 Jun 2021 20:08:08 GMT, Michael Bien 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 216:
> 
>> 214: }
>> 215: 
>> 216: return super.transferTo(out);
> 
> (i am no reviewer)
> wouldn't this method be more intuitive if it wouldn't try to avoid using 
> "else if" and "else"? If there are sequential if blocks with return in them, 
> part of me is always trying to find the fall-through scenario, but in this 
> case its all distinct code paths. Using branches would make that obvious + if 
> there would be a fall through in future, the compiler would generate a 
> "missing return" error right away. 
> 
> example:
> 
> if (out instanceof ChannelOutputStream cos) {
> 
> WritableByteChannel oc = cos.channel();
> long i = 0L;
> 
> if (ch instanceof FileChannel fc) {
> 
> long pos = fc.position();
> long size = fc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferTo(pos + i, Long.MAX_VALUE, oc);
> return i;
> } finally {
> fc.position(pos + i);
> }
> 
> } else if (oc instanceof FileChannel fc) {
> 
> long fcpos = fc.position();
> 
> if (ch instanceof SeekableByteChannel sbc) {
> 
> long pos = sbc.position();
> long size = sbc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferFrom(ch, fcpos + i, 
> Long.MAX_VALUE);
> return i;
> } finally {
> fc.position(fcpos + i);
> }
> 
> } else {
> 
> ByteBuffer bb = 
> Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> int r;
> do {
> i += fc.transferFrom(ch, fcpos + i, 
> Long.MAX_VALUE);
> r = ch.read(bb); // detect end-of-stream
> if (r > -1) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> } while (r > -1);
> return i;
> } finally {
> fc.position(fcpos + i);
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
> 
> } else {
> 
> ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> for (int r = ch.read(bb); r > -1; r = ch.read(bb)) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> return i;
> } finally {
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
> } else {
> return su

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Adding an override of transferTo may require new tests. @bplb Do you if we have 
good tests for all the scenarios where input stream returned by 
Channels.newInputStream is the source?

I think we also need to decide if this PR is about Channels.newInputStream or 
the input stream return by Files.newInputStream as the latter could return its 
own input stream implementation, it doesn't need to ChannelInputStream.

If the approach on the table goes ahead then ChannelInputStream.transferTo can 
be split into 4 simple methods to make it easier to maintain. You can use 
something like "total" or "bytesWritten" for the total number of bytes written 
rather than "i".

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Brian Burkhalter]

On Jun 18, 2021, at 6:36 AM, Alan Bateman 
mailto:al...@openjdk.java.net>> wrote:

Adding an override of transferTo may require new tests. @bplb Do you if we have 
good tests for all the scenarios where input stream returned by 
Channels.newInputStream is the source?

There are not a lot of tests for the `InputStream` returned by 
`Channels.newInputStream`:

 `java/nio/channels/Channels`:

`Basic.java` - reading from a wrapped `FileChannel`
`ReadByte.java` - trivial
`Basic2.java` - reading at random offsets from a wrapped 
`AsynchronousSocketChannel`
`ReadOffset.java` - trivial

Elsewhere:

`sun/nio/ch/TempBuffer.java`
`jdk/nio/zipfs/ZipFSTester.java`


I don’t see that `transferTo()` on such a stream is tested anywhere.

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On 18/06/2021 18:30, Brian Burkhalter wrote:


On Jun 18, 2021, at 6:36 AM, Alan Bateman > wrote:


Adding an override of transferTo may require new tests. @bplb Do you 
if we have good tests for all the scenarios where input stream 
returned by Channels.newInputStream is the source?


There are not a lot of tests for the `InputStream` returned by 
`Channels.newInputStream`:


 `java/nio/channels/Channels`:

`Basic.java` - reading from a wrapped `FileChannel`
`ReadByte.java` - trivial
`Basic2.java` - reading at random offsets from a wrapped 
`AsynchronousSocketChannel`

`ReadOffset.java` - trivial

Elsewhere:

`sun/nio/ch/TempBuffer.java`
`jdk/nio/zipfs/ZipFSTester.java`


I don’t see that `transferTo()` on such a stream is tested anywhere.


InputStream/TransferTo.java tests the default implementation. Maybe we 
re-structured this to use a data provider so that the tests exercise 
both the default implementation and the various overrides. Alternatively 
a new test that exercises the various overrides in different scenarios. 
In any case, I think the feedback for PR 4263 is that it is essentially 
several implementations of transferTo and each of those need to have 
test coverage.


-Alan

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Markus KARG]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

I am a bit lost currently. So what actually do I have to do next? You want *me* 
to write these tests, or shall I just refactor my implementation and *you* will 
provide these tests (I would prefer that, actually)?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

[Alan Bateman]

On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 
 wrote:

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

> I am a bit lost currently. So what actually do I have to do next? You want 
> _me_ to write these tests, or shall I just refactor my implementation and 
> _you_ will provide these tests (I would prefer that, actually)?

The existing test for InputStream.transferTo mostly exercises the default 
implementation. There are overrides in other input stream implementation but 
the test coverage appears to be spotty. This PR adds an overwrite with four 
implementations and it doesn't appear that these are exercised by any tests. So 
yes, tests are needed. This PR, or another PR that is integrated in advance, 
either is fine.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v2]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  Draft: Renaming i and separating code into several methods
  
  Signed-off-by: Markus Karg 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/ed49098a..7d18ca62

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=00-01

  Stats: 108 lines in 1 file changed: 57 ins; 39 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Draft: Renaming i and separating code into several methods
  
  Signed-off-by: Markus Karg 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/7d18ca62..47ee00a2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=01-02

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v2]

[Markus KARG]

On Thu, 1 Jul 2021 21:59:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Draft: Renaming i and separating code into several methods
>   
>   Signed-off-by: Markus Karg 

I have renamed `i` (and other ambiguous variables) and separated the 
implementation of `transferTo` into multiple methods. I will look into the test 
examples next to learn what I can reuse when writing tests for all those 
implementations. Thank you for all the kind input and the good ideas.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

I cloned the `InputStream/TransferTo` test in a way which uses providers, but 
when started to implement the providers I noticed that a huge amount of *empty* 
source code is needed just to make the compiler happy: `FileChannel` already 
has dozens of abstract methods I have to override but they never will be used 
by any of the tests... Having that said, it would be good if I would be allowed 
to spare us thousands of useless codelines:
* May I just implement the `content()` test or do you insist on me really 
implementing *all* the tests found in `InputStream/TransferTo` for *each* of 
the overloaded `transferTo` implementations?
* May I use a mocking framework to actually just provide *partial* 
implementations of the channels, or do you insist on me actually overloading 
*all* methods of *all* channels in actual Java source code just for the sake of 
making the compiler happy?

While I understand the necessity of tests, and while I am convinced that the 
performance benefit outweighs the weeks of work this would imply just for 
adding all those test code, I actually like to propose that I only cover the 
`content()` tests plus using Mockito, so we would have 80% of the benefit for 
20% of the coding costs.

WDYT?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Brian Burkhalter]

On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   Draft: Renaming i and separating code into several methods
>   
>   Signed-off-by: Markus Karg 

Mockito is not approved for distribution and/or hosting by Java (Oracle JDK / 
OpenJDK).

I understand not wanting to have a lot of useless code. Have you looked into 
other tests which have implemented Providers?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

On Wed, 14 Jul 2021 18:35:15 GMT, Brian Burkhalter  wrote:

> Have you looked into other tests which have implemented Providers?

I have not found any test code containing the word `new FileChannel() {...}` or 
`extends FileChannel`, so I apparently there exists no mock of `FileChannel`. 
Unfortunately, as long as it is mandatory to copy *all* the tests of 
`InputStream/TransferTo` this means that I *must* implement *all* methods of 
`FileChannel` (even if they are empty) just for the sake of throwing an 
exception after one, two, or three transferred bytes (which, BTW, is not part 
of the API but looks like just a personal decision of the original test 
author). It is not really helpful that OpenJDK *neither* uses mocking *nor* 
lets me reduce the number of tests to the *essential* minimum, as this means, I 
wrote approx. 100 LoC for the optimized implementations and now I have to write 
approx. 1000 LoC just to perform *exactly the same tests* (to be clear: I have 
no problem with testing `content()` which certainly *has* to be tested, but 
which does *not* imply writing a `FileChannel` mock).

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   Draft: Renaming i and separating code into several methods
>   
>   Signed-off-by: Markus Karg 

Understood. Work is in progress. Stay tuned.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v4]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with four additional 
commits since the last revision:

 - Draft: Test for ChannelInputStream::transferTo
 - Draft: Using Channels API to invoke sun.nio.ch classes
 - Draft: MUST check params before ANY other operation
 - Draft: ChannelInputStream::transferTo Test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/47ee00a2..b431fcd6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=02-03

  Stats: 229 lines in 2 files changed: 229 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

Sorry for the delay. I now have pushed a first draft of the test. IIUC then 
five implementation have to be tested just for the actualy API, i. e. whether 
they throws NPE when `out` is `null`, and whether they transfer small, single 
turn, and multiple turn loads correctly. The tests are taken from 
`InputStream/TransferTo.java` as you proposed, and I am not using mocking but 
instead use `Channels.new*` as the existing tests you proposed as a blueprint. 
I hope I understood your requests and proposals correctly, if not please tell 
me. The tests pass well and proof that the new implementations work as expected 
and API-compliant.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v5]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Draft: Test for ChannelInputStream::transferTo

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/b431fcd6..bfc699a0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=03-04

  Stats: 227 lines in 1 file changed: 0 ins; 0 del; 227 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Draft: Test for ChannelInputStream::transferTo

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/bfc699a0..1f9dba3d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=04-05

  Stats: 173 lines in 1 file changed: 6 ins; 0 del; 167 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Mon, 26 Jul 2021 20:45:14 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179:

> 177: for (long n = srcSize - srcPos; bytesWritten < n;)
> 178: bytesWritten += src.transferTo(srcPos + bytesWritten, 
> Long.MAX_VALUE, dest);
> 179: return bytesWritten;

If `src` is extended at an inconvenient point in time, for example between the 
return from a call to `src.transferTo()` that makes `bytesWritten < n` false 
and the evaluation of that terminating condition, then it appears that not all 
the content of `src` will be transferred to `dest`. I am not however sure that 
this violates any contract but it could be a behavioral change from the 
existing implementation.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Mon, 26 Jul 2021 20:45:14 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85:

> 83: private byte[] bs;   // Invoker's previous array
> 84: private byte[] b1;
> 85: 

It might be better to put the field declarations at the beginning of the class 
before the static methods.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Mon, 26 Jul 2021 20:45:14 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67:

> 65: test(readableByteChannelInput(), defaultOutput());
> 66: }
> 67: 

This test looks like it's doing what it's supposed to do. I'm not asking to 
change it, but I think using TestNG might have given a simpler result with less 
work. For example, there could be one `DataProvider` supplying inputs which 
feed a `@Test` which expects an NPE, and another `DataProvider` supplying 
input-output pairs which feeds a `@Test` which validates the functionality.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Tue, 27 Jul 2021 00:57:02 GMT, Brian Burkhalter  wrote:

>> Markus KARG has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR.
>
> src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85:
> 
>> 83: private byte[] bs;   // Invoker's previous array
>> 84: private byte[] b1;
>> 85: 
> 
> It might be better to put the field declarations at the beginning of the 
> class before the static methods.

This is a question of style and personal likes. Code maintenance is much easier 
if variables are defined *near* to its first use, not *far away* (as in the 
Pascal or C language). If the reviewers want me to change it, I will do it.

> test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67:
> 
>> 65: test(readableByteChannelInput(), defaultOutput());
>> 66: }
>> 67: 
> 
> This test looks like it's doing what it's supposed to do. I'm not asking to 
> change it, but I think using TestNG might have given a simpler result with 
> less work. For example, there could be one `DataProvider` supplying inputs 
> which feed a `@Test` which expects an NPE, and another `DataProvider` 
> supplying input-output pairs which feeds a `@Test` which validates the 
> functionality.

No doubt, using modern tools would have spared me work, and indeed I would have 
chosen JUnit or TestNG if there would be a clear commitment to that tool 
*upfront*. For now, I see now use in reworking the code afterwards.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Mon, 26 Jul 2021 23:59:05 GMT, Brian Burkhalter  wrote:

>> Markus KARG has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR.
>
> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179:
> 
>> 177: for (long n = srcSize - srcPos; bytesWritten < n;)
>> 178: bytesWritten += src.transferTo(srcPos + bytesWritten, 
>> Long.MAX_VALUE, dest);
>> 179: return bytesWritten;
> 
> If `src` is extended at an inconvenient point in time, for example between 
> the return from a call to `src.transferTo()` that makes `bytesWritten < n` 
> false and the evaluation of that terminating condition, then it appears that 
> not all the content of `src` will be transferred to `dest`. I am not however 
> sure that this violates any contract but it could be a behavioral change from 
> the existing implementation.

The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that there 
is **no** guarantee of *any* specific behavior in that particular case: 
>The behavior for the case where the input and/or output stream is 
>asynchronously closed, or the thread interrupted during the transfer, is 
>highly input and output stream specific, and therefore not specified.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Thu, 29 Jul 2021 08:12:23 GMT, Markus KARG 
 wrote:

>> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179:
>> 
>>> 177: for (long n = srcSize - srcPos; bytesWritten < n;)
>>> 178: bytesWritten += src.transferTo(srcPos + bytesWritten, 
>>> Long.MAX_VALUE, dest);
>>> 179: return bytesWritten;
>> 
>> If `src` is extended at an inconvenient point in time, for example between 
>> the return from a call to `src.transferTo()` that makes `bytesWritten < n` 
>> false and the evaluation of that terminating condition, then it appears that 
>> not all the content of `src` will be transferred to `dest`. I am not however 
>> sure that this violates any contract but it could be a behavioral change 
>> from the existing implementation.
>
> The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that there 
> is **no** guarantee of *any* specific behavior in that particular case: 
>>The behavior for the case where the input and/or output stream is 
>>asynchronously closed, or the thread interrupted during the transfer, is 
>>highly input and output stream specific, and therefore not specified.

Good point.

>> src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85:
>> 
>>> 83: private byte[] bs;   // Invoker's previous array
>>> 84: private byte[] b1;
>>> 85: 
>> 
>> It might be better to put the field declarations at the beginning of the 
>> class before the static methods.
>
> This is a question of style and personal likes. Code maintenance is much 
> easier if variables are defined *near* to its first use, not *far away* (as 
> in the Pascal or C language). If the reviewers want me to change it, I will 
> do it.

It's actually a matter of convention but I think it can remain as it is.

>> test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67:
>> 
>>> 65: test(readableByteChannelInput(), defaultOutput());
>>> 66: }
>>> 67: 
>> 
>> This test looks like it's doing what it's supposed to do. I'm not asking to 
>> change it, but I think using TestNG might have given a simpler result with 
>> less work. For example, there could be one `DataProvider` supplying inputs 
>> which feed a `@Test` which expects an NPE, and another `DataProvider` 
>> supplying input-output pairs which feeds a `@Test` which validates the 
>> functionality.
>
> No doubt, using modern tools would have spared me work, and indeed I would 
> have chosen JUnit or TestNG if there would be a clear commitment to that tool 
> *upfront*. For now, I see now use in reworking the code afterwards.

No need to rework it now.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Thu, 29 Jul 2021 16:42:35 GMT, Brian Burkhalter  wrote:

>> The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that 
>> there is **no** guarantee of *any* specific behavior in that particular 
>> case: 
>>>The behavior for the case where the input and/or output stream is 
>>>asynchronously closed, or the thread interrupted during the transfer, is 
>>>highly input and output stream specific, and therefore not specified.
>
> Good point.

:-)

>> This is a question of style and personal likes. Code maintenance is much 
>> easier if variables are defined *near* to its first use, not *far away* (as 
>> in the Pascal or C language). If the reviewers want me to change it, I will 
>> do it.
>
> It's actually a matter of convention but I think it can remain as it is.

Ok for me, otherwise just clearly tell me and I do change it.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Thu, 29 Jul 2021 19:56:40 GMT, Markus KARG 
 wrote:

>> It's actually a matter of convention but I think it can remain as it is.
>
> Ok for me, otherwise just clearly tell me and I do change it.

I think you can leave it unless someone else thinks otherwise.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Alan Bateman]

On Thu, 29 Jul 2021 19:57:30 GMT, Markus KARG 
 wrote:

>> Good point.
>
> :-)

Is this loop correct for the case that the channel gets truncated? In that case 
transferTo will return 0 as no bytes will be transferred and I'm concerned this 
code will go into an infinite loop.

Also can you can check that IllegalBlockingModeException is thrown for the case 
ch is a SelectableChannel configured non-blocking and the destination is a 
FileChannel?

Just on naming, the existing channel implementations use "dst" for the 
destination and "wbc" (not "oc") for writable byte channels. Just mentioning it 
so that the new code can be kept consistent where possible.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Fri, 30 Jul 2021 12:09:30 GMT, Alan Bateman  wrote:

> Just on naming, the existing channel implementations use "dst" for the 
> destination and "wbc" (not "oc") for writable byte channels. Just mentioning 
> it so that the new code can be kept consistent where possible.

I have renamed `dest` to `dst` and `oc` to `wbc` in 
https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v7]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  Draft: Renamed oc to wbc and dest to dst

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/1f9dba3d..f91d5422

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=05-06

  Stats: 17 lines in 1 file changed: 0 ins; 0 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v8]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with two additional 
commits since the last revision:

 - Draft: Renamed r to bytesRead
 - Draft: Prevent infinite loop after truncation

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/f91d5422..f3e37fe1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=06-07

  Stats: 16 lines in 1 file changed: 2 ins; 2 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v9]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains two new commits 
since the last revision:

 - Draft: Renamed r to bytesRead
 - Draft: Prevent infinite loop after truncation

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/f3e37fe1..e0724718

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=07-08

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sat, 31 Jul 2021 13:12:54 GMT, Markus KARG 
 wrote:

>> Is this loop correct for the case that the channel gets truncated? In that 
>> case transferTo will return 0 as no bytes will be transferred and I'm 
>> concerned this code will go into an infinite loop.
>> 
>> Also can you can check that IllegalBlockingModeException is thrown for the 
>> case ch is a SelectableChannel configured non-blocking and the destination 
>> is a FileChannel?
>> 
>> Just on naming, the existing channel implementations use "dst" for the 
>> destination and "wbc" (not "oc") for writable byte channels. Just mentioning 
>> it so that the new code can be kept consistent where possible.
>
>> Just on naming, the existing channel implementations use "dst" for the 
>> destination and "wbc" (not "oc") for writable byte channels. Just mentioning 
>> it so that the new code can be kept consistent where possible.
> 
> I have renamed `dest` to `dst` and `oc` to `wbc` in 
> https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a.

> Is this loop correct for the case that the channel gets truncated? In that 
> case transferTo will return 0 as no bytes will be transferred and I'm 
> concerned this code will go into an infinite loop.

Good catch, indeed missed this particular situation!

The modified code found in 
https://github.com/openjdk/jdk/pull/4263/commits/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20
 should be safe from infinite loops, as it checks the actual file length in 
each iteration (even if this costs us one more `synchronized` per iteration).

This will also improve the situation with concurrent *extension* of the file as 
outlined in:

>If src is extended at an inconvenient point in time, for example between the 
>return from a call to src.transferTo() that makes bytesWritten < n false and 
>the evaluation of that terminating condition, then it appears that not all the 
>content of src will be transferred to dest. I am not however sure that this 
>violates any contract but it could be a behavioral change from the existing 
>implementation.

As `size()` is called in each iteration, it will pick up the appendend bytes. 
Still the situation exists that some bytes are *replaced* in the source file -- 
but that is actually out-of-scope of this PR...! ;-)

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v10]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  Draft: Throwing IllegalBlockingModeException if src is SelectableChannel and 
dst is FileChannel

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/e0724718..8e2889fd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=08-09

  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sat, 31 Jul 2021 14:39:02 GMT, Markus KARG 
 wrote:

>>> Just on naming, the existing channel implementations use "dst" for the 
>>> destination and "wbc" (not "oc") for writable byte channels. Just 
>>> mentioning it so that the new code can be kept consistent where possible.
>> 
>> I have renamed `dest` to `dst` and `oc` to `wbc` in 
>> https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a.
>
>> Is this loop correct for the case that the channel gets truncated? In that 
>> case transferTo will return 0 as no bytes will be transferred and I'm 
>> concerned this code will go into an infinite loop.
> 
> Good catch, indeed missed this particular situation!
> 
> The modified code found in 
> https://github.com/openjdk/jdk/pull/4263/commits/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20
>  should be safe from infinite loops, as it checks the actual file length in 
> each iteration (even if this costs us one more `synchronized` per iteration).
> 
> This will also improve the situation with concurrent *extension* of the file 
> as outlined in:
> 
>>If src is extended at an inconvenient point in time, for example between the 
>>return from a call to src.transferTo() that makes bytesWritten < n false and 
>>the evaluation of that terminating condition, then it appears that not all 
>>the content of src will be transferred to dest. I am not however sure that 
>>this violates any contract but it could be a behavioral change from the 
>>existing implementation.
> 
> As `size()` is called in each iteration, it will pick up the appendend bytes. 
> Still the situation exists that some bytes are *replaced* in the source file 
> -- but that is actually out-of-scope of this PR...! ;-)

> Also can you can check that IllegalBlockingModeException is thrown for the 
> case ch is a SelectableChannel configured non-blocking and the destination is 
> a FileChannel?

Done in 
https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7,
 but honestly I'd kindly like to ask for a brief explanation why this has to be 
done. What actual bad effect would happen if I do not throw?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Alan Bateman]

On Sat, 31 Jul 2021 15:58:07 GMT, Markus KARG 
 wrote:

>>> Is this loop correct for the case that the channel gets truncated? In that 
>>> case transferTo will return 0 as no bytes will be transferred and I'm 
>>> concerned this code will go into an infinite loop.
>> 
>> Good catch, indeed missed this particular situation!
>> 
>> The modified code found in 
>> https://github.com/openjdk/jdk/pull/4263/commits/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20
>>  should be safe from infinite loops, as it checks the actual file length in 
>> each iteration (even if this costs us one more `synchronized` per iteration).
>> 
>> This will also improve the situation with concurrent *extension* of the file 
>> as outlined in:
>> 
>>>If src is extended at an inconvenient point in time, for example between the 
>>>return from a call to src.transferTo() that makes bytesWritten < n false and 
>>>the evaluation of that terminating condition, then it appears that not all 
>>>the content of src will be transferred to dest. I am not however sure that 
>>>this violates any contract but it could be a behavioral change from the 
>>>existing implementation.
>> 
>> As `size()` is called in each iteration, it will pick up the appendend 
>> bytes. Still the situation exists that some bytes are *replaced* in the 
>> source file -- but that is actually out-of-scope of this PR...! ;-)
>
>> Also can you can check that IllegalBlockingModeException is thrown for the 
>> case ch is a SelectableChannel configured non-blocking and the destination 
>> is a FileChannel?
> 
> Done in 
> https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7,
>  but honestly I'd kindly like to ask for a brief explanation why this has to 
> be done. What actual bad effect would happen if I do not throw?

> The modified code found in 
> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20)
>  should be safe from infinite loops, as it checks the actual file length in 
> each iteration (even if this costs us one more `synchronized` per iteration).

I need to look at it closely but I suspect this introduces a potential 
overflow. Also if output stream is backed by a SocketChannel configured 
non-blocking then FC::transferTo may return 0 so I assume there is a potential 
infinite loop there too. I suspect the eventually patch will need have to make 
use of the blockingLock to prevent the underlying channels from being changed 
to non-blocking during the transfer.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sat, 31 Jul 2021 17:33:50 GMT, Alan Bateman  wrote:

> I need to look at it closely but I suspect this introduces a potential 
> overflow. Also if output stream is backed by a SocketChannel configured 
> non-blocking then FC::transferTo may return 0 so I assume there is a 
> potential infinite loop there too. I suspect the eventually patch will need 
> have to make use of the blockingLock to prevent the underlying channels from 
> being changed to non-blocking during the transfer.

I need to confess that my NIO knowledge is not deep enough to follow you 
closely, so I trust on your advice how to go on from here.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v11]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  [Draft] Using blocking lock to prevent concurrently switching into 
non-blocking mode

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/8e2889fd..f4485d5b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=09-10

  Stats: 16 lines in 1 file changed: 8 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sat, 31 Jul 2021 21:20:28 GMT, Markus KARG 
 wrote:

> I suspect the eventually patch will need have to make use of the blockingLock 
> to prevent the underlying channels from being changed to non-blocking during 
> the transfer.

The blocking lock now is used since 
https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac
 to prevent this.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sun, 1 Aug 2021 07:46:36 GMT, Markus KARG 
 wrote:

>>> I need to look at it closely but I suspect this introduces a potential 
>>> overflow. Also if output stream is backed by a SocketChannel configured 
>>> non-blocking then FC::transferTo may return 0 so I assume there is a 
>>> potential infinite loop there too. I suspect the eventually patch will need 
>>> have to make use of the blockingLock to prevent the underlying channels 
>>> from being changed to non-blocking during the transfer.
>> 
>> I need to confess that my NIO knowledge is not deep enough to follow you 
>> closely, so I trust on your advice how to go on from here.
>
>> I suspect the eventually patch will need have to make use of the 
>> blockingLock to prevent the underlying channels from being changed to 
>> non-blocking during the transfer.
> 
> The blocking lock now is used since 
> https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac
>  to prevent this.

> Also can you can check that IllegalBlockingModeException is thrown for the 
> case ch is a SelectableChannel configured non-blocking and the destination is 
> a FileChannel?

Do we really only have to check this in the particular case of `FileChannel` 
destinations?

And don't we have to assert blocking mode for *destination* channels, too (just 
like `ChannelOutputStream::writeFully` does)?

As this results in an 2:2 matrix of possibilities (src is selectable nor not, 
dst is selectable or not) it might be easier to do *only the blocking checks* 
in `transferTo` but let it call something like 
`transferFromSelectableToNonSelectable` in turn *or* to wrap *each* 
implementation of `transferTo` in a wrapper like `executeInBlockingMode((src, 
dst) -> transferTo(src, dst))`...?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sat, 31 Jul 2021 17:33:50 GMT, Alan Bateman  wrote:

>>> Also can you can check that IllegalBlockingModeException is thrown for the 
>>> case ch is a SelectableChannel configured non-blocking and the destination 
>>> is a FileChannel?
>> 
>> Done in 
>> https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7,
>>  but honestly I'd kindly like to ask for a brief explanation why this has to 
>> be done. What actual bad effect would happen if I do not throw?
>
>> The modified code found in 
>> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20)
>>  should be safe from infinite loops, as it checks the actual file length in 
>> each iteration (even if this costs us one more `synchronized` per iteration).
> 
> I need to look at it closely but I suspect this introduces a potential 
> overflow. Also if output stream is backed by a SocketChannel configured 
> non-blocking then FC::transferTo may return 0 so I assume there is a 
> potential infinite loop there too. I suspect the eventually patch will need 
> have to make use of the blockingLock to prevent the underlying channels from 
> being changed to non-blocking during the transfer.

@AlanBateman Did I recap the sum your comments correctly, is the above 
conclusion what you wanted to tell me? Shall I proceed with one of the two 
solutions outlined in the "...2:2 matrix..." section of my answer *or* shall I 
wait until you had a deeper look?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v12]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  [Draft] Using blocking lock to prevent concurrently switching into 
non-blocking mode

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/f4485d5b..55c96880

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10-11

  Stats: 16 lines in 1 file changed: 8 ins; 6 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with two additional 
commits since the last revision:

 - Draft: Eliminated duplicate code using lambda expressions
 - Draft: Use blocking mode also for target channel

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/55c96880..4a0d7cf7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11-12

  Stats: 43 lines in 1 file changed: 21 ins; 8 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Markus KARG]

On Sun, 1 Aug 2021 08:24:13 GMT, Markus KARG 
 wrote:

>>> The modified code found in 
>>> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20)
>>>  should be safe from infinite loops, as it checks the actual file length in 
>>> each iteration (even if this costs us one more `synchronized` per 
>>> iteration).
>> 
>> I need to look at it closely but I suspect this introduces a potential 
>> overflow. Also if output stream is backed by a SocketChannel configured 
>> non-blocking then FC::transferTo may return 0 so I assume there is a 
>> potential infinite loop there too. I suspect the eventually patch will need 
>> have to make use of the blockingLock to prevent the underlying channels from 
>> being changed to non-blocking during the transfer.
>
> @AlanBateman Did I recap the sum your comments correctly, is the above 
> conclusion what you wanted to tell me? Shall I proceed with one of the two 
> solutions outlined in the "...2:2 matrix..." section of my answer *or* shall 
> I wait until you had a deeper look?

Asserting blocking mode for *both* sides (source and target) in 
https://github.com/openjdk/jdk/pull/4263/commits/fc38eae44de9e16468d33bd2ebab6502c92b4860.

Eliminated the resulting duplicate code in 
https://github.com/openjdk/jdk/pull/4263/commits/4a0d7cf74ee7e35aa0448df4b5ea4c5e3113ece6.

Do you see more problems we need to solve?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

I think I fixed all requested changes. Anymore comments on this PR?

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Alan Bateman]

On Sat, 7 Aug 2021 06:45:21 GMT, Markus KARG 
 wrote:

> I think I fixed all requested changes. Anymore comments on this PR?

I hope to get to this soon.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Alan Bateman]

On Mon, 9 Aug 2021 18:10:52 GMT, Alan Bateman  wrote:

> I think I fixed all requested changes. Anymore comments on this PR?

I've looked through the latest revision. Is here any way that we could drop 
most of the changes to ChannelInputStream and focus on one or two specific 
cases? I'm asking because there are several issues, inconsistencies, and it is 
trying to cover many scenarios that aren't covered by the test.

If the original motivation was file -> file then it could be simplified down to 
a FileChannel -> FileChannel transfer as the default provider uses file 
channels.  We could even push some support into FileChannelImpl so that it is 
done while holding the position lock.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Wed, 11 Aug 2021 11:27:53 GMT, Alan Bateman  wrote:

> I've looked through the latest revision. Is there any way that we could drop 
> most of the changes to ChannelInputStream and focus on one or two specific 
> cases? I'm asking because there are several issues, inconsistencies, and it 
> is trying to cover many scenarios that aren't covered by the test.
> If the original motivation was file -> file then it could be simplified down 
> to a FileChannel -> FileChannel transfer as the default provider uses file 
> channels. We could even push some support into FileChannelImpl so that it is 
> done while holding the position lock.

I am a bit disappointed actually about that destructive answer at that late 
point in time, now that I worked for months on all the requested changes and 
tests. To prevent exactly this situation, I deliberately had the discussion 
started in JIRA only, and I deliberately had the original code being just a 
draft in the first place, and I deliberately did nearly *everything* I was 
asked to (including even the most irrelevant minor code style issues). And you 
come up with the request to drop the code **now**?

Certainly we could reduce the PR to just file channels, but in fact, now that I 
spent all the time in the non-file-channels, I wonder why I shall throw away 
all that work and go just with file channels actually? What is not covered that 
was originally covered, and what is that lots of issues you talk about? 
Actually I cannot see the actual problem unless you name it.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Brian Burkhalter]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 175:

> 173: throw new IllegalBlockingModeException();
> 174: }
> 175: return tls.supply();

It does not look like the `SelectableChannel` branch is tested, including 
whether an `IllegalBlockingModeException` is thrown when it should be.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 249:

> 247: }
> 248: 
> 249: private static long transfer(ReadableByteChannel src, 
> WritableByteChannel dst) throws IOException {

Does this method have a measurable improvement in performance over 
`InputStream.transferTo()`?

test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 93:

> 91: checkTransferredContents(inputStreamProvider, 
> outputStreamProvider, createRandomBytes(1024, 4096));
> 92: // to span through several batches
> 93: checkTransferredContents(inputStreamProvider, 
> outputStreamProvider, createRandomBytes(16384, 16384));

Should some random-sized buffers be tested? (Use `jdk.test.lib.RandomFactory` 
factory for this, not `j.u.Random`. The existing use of `Random` is fine.)

Should some testing be done of streams with non-zero initial position?

test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 101:

> 99: try (InputStream in = inputStreamProvider.input(inBytes);
> 100: OutputStream out = 
> outputStreamProvider.output(recorder::set)) {
> 101: in.transferTo(out);

The return value of `transferTo()` is not checked.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Brian Burkhalter]

On Wed, 11 Aug 2021 11:27:53 GMT, Alan Bateman  wrote:

>>> I think I fixed all requested changes. Anymore comments on this PR?
>> 
>> I hope to get to this soon.
>
>> I think I fixed all requested changes. Anymore comments on this PR?
> 
> I've looked through the latest revision. Is there any way that we could drop 
> most of the changes to ChannelInputStream and focus on one or two specific 
> cases? I'm asking because there are several issues, inconsistencies, and it 
> is trying to cover many scenarios that aren't covered by the test.
> 
> If the original motivation was file -> file then it could be simplified down 
> to a FileChannel -> FileChannel transfer as the default provider uses file 
> channels.  We could even push some support into FileChannelImpl so that it is 
> done while holding the position lock.

I do not know exactly what @AlanBateman had in mind, but I think there is 
general concern about ensuring that all combinations of channel types and all 
execution paths are exercised.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Alan Bateman]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

> I am a bit disappointed actually about that destructive answer at that late 
> point in time, now that I worked for months on all the requested changes and 
> tests. To prevent exactly this situation, I deliberately had the discussion 
> started in JIRA only, and I deliberately had the original code being just a 
> draft in the first place, and I deliberately did nearly _everything_ I was 
> asked to (including even the most irrelevant minor code style issues). And 
> you come up with the request to drop the code **now**?
> 
> Certainly we could reduce the PR to just file channels, but in fact, now that 
> I spent all the time in the non-file-channels, I wonder why I shall throw 
> away all that work and go just with file channels actually? What is not 
> covered that was originally covered, and what is that lots of issues you talk 
> about? Actually I cannot see the actual problem unless you name it.

There are 78 comments on this PR so far. We've tried to point out the bugs and 
issues at each iteration. We asked for tests because the changes introduce 
several code paths and implementations that would not be exercised by existing 
tests. There are several scenarios still missing and the patch doesn't yet have 
the microbenchmarks to demonstrate the improvements.

I assume this is your first contribution so there will be learning curve and 
maybe some frustration. I think you have a better chance of success if you 
split this up and reduce the scope of this PR down to something manageable. 
Keeping the selectable channels out of this PR and focusing on the case where 
the input and output streams wrap file channels should make it simpler and may 
lead to a better solution. Reducing the scope will also reduce the burden on 
reviewers.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Thu, 12 Aug 2021 08:40:34 GMT, Alan Bateman  wrote:

>> Markus KARG has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Draft: Eliminated duplicate code using lambda expressions
>>  - Draft: Use blocking mode also for target channel
>
>> I am a bit disappointed actually about that destructive answer at that late 
>> point in time, now that I worked for months on all the requested changes and 
>> tests. To prevent exactly this situation, I deliberately had the discussion 
>> started in JIRA only, and I deliberately had the original code being just a 
>> draft in the first place, and I deliberately did nearly _everything_ I was 
>> asked to (including even the most irrelevant minor code style issues). And 
>> you come up with the request to drop the code **now**?
>> 
>> Certainly we could reduce the PR to just file channels, but in fact, now 
>> that I spent all the time in the non-file-channels, I wonder why I shall 
>> throw away all that work and go just with file channels actually? What is 
>> not covered that was originally covered, and what is that lots of issues you 
>> talk about? Actually I cannot see the actual problem unless you name it.
> 
> There are 78 comments on this PR so far. We've tried to point out the bugs 
> and issues at each iteration. We asked for tests because the changes 
> introduce several code paths and implementations that would not be exercised 
> by existing tests. There are several scenarios still missing and the patch 
> doesn't yet have the microbenchmarks to demonstrate the improvements.
> 
> I assume this is your first contribution so there will be learning curve and 
> maybe some frustration. I think you have a better chance of success if you 
> split this up and reduce the scope of this PR down to something manageable. 
> Keeping the selectable channels out of this PR and focusing on the case where 
> the input and output streams wrap file channels should make it simpler and 
> may lead to a better solution. Reducing the scope will also reduce the burden 
> on reviewers.

> I do not know exactly what @AlanBateman had in mind, but I think there is 
> general concern about ensuring that all combinations of channel types and all 
> execution paths are exercised.

@AlanBateman @bplb 

Does it make **any** real sense to answer your recent questions, provide the 
proofs, tests and benchmark results (I actually would love to *if* it makes 
sense) *or* will the outcome be that I *must* drop everything besides file 
channels *anyways* (In that case it is in vain)? As my time is just as precious 
as yours I really need to know that **before** I spend more weeks into code 
paths that you possibly decided to never accept ever. Don't get me wrong, if 
you see a chance to keep the code once I provided the answers I will do that, 
but if you do not see that chance, please frankly and unambiguously tell me 
**now**. Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Alan Bateman]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

> Does it make **any** real sense to answer your recent questions, provide the 
> proofs, tests and benchmark results (I actually would love to _if_ it makes 
> sense) _or_ will the outcome be that I _must_ drop everything besides file 
> channels _anyways_ (In that case it is in vain)? As my time is just as 
> precious as yours I really need to know that **before** I spend more weeks 
> into code paths that you possibly decided to never accept ever. Don't get me 
> wrong, if you see a chance to keep the code once I provided the answers I 
> will do that, but if you do not see that chance, please frankly and 
> unambiguously tell me **now**. Thanks.

I think the best course of action is to reduce the scope of this PR to the file 
channel cases. There is no reason why future PRs can't build on this and add 
implementations for other channel types.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Fri, 13 Aug 2021 12:54:55 GMT, Alan Bateman  wrote:

> > Does it make **any** real sense to answer your recent questions, provide 
> > the proofs, tests and benchmark results (I actually would love to _if_ it 
> > makes sense) _or_ will the outcome be that I _must_ drop everything besides 
> > file channels _anyways_ (In that case it is in vain)? As my time is just as 
> > precious as yours I really need to know that **before** I spend more weeks 
> > into code paths that you possibly decided to never accept ever. Don't get 
> > me wrong, if you see a chance to keep the code once I provided the answers 
> > I will do that, but if you do not see that chance, please frankly and 
> > unambiguously tell me **now**. Thanks.
> 
> I think the best course of action is to reduce the scope of this PR to the 
> file channel cases. There is no reason why future PRs can't build on this and 
> add implementations for other channel types.

Agreed, I will split this PR into several PRs, so we can start with the 
low-hanging fruits first and later dive into the more complex cases.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 15 Aug 2021 13:37:23 GMT, Markus KARG 
 wrote:

> I think the best course of action is to reduce the scope of this PR to the 
> file channel cases. There is no reason why future PRs can't build on this and 
> add implementations for other channel types.

I have split up this PR so that only the lowest hanging fruit is covered and 
kindly request review: https://github.com/openjdk/jdk/pull/5179.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Thu, 12 Aug 2021 01:10:15 GMT, Brian Burkhalter  wrote:

>> Markus KARG has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Draft: Eliminated duplicate code using lambda expressions
>>  - Draft: Use blocking mode also for target channel
>
> test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 93:
> 
>> 91: checkTransferredContents(inputStreamProvider, 
>> outputStreamProvider, createRandomBytes(1024, 4096));
>> 92: // to span through several batches
>> 93: checkTransferredContents(inputStreamProvider, 
>> outputStreamProvider, createRandomBytes(16384, 16384));
> 
> Should some random-sized buffers be tested? (Use `jdk.test.lib.RandomFactory` 
> factory for this, not `j.u.Random`. The existing use of `Random` is fine.)
> 
> Should some testing be done of streams with non-zero initial position?

Done in https://github.com/openjdk/jdk/pull/5209, will rebase on that PR once 
merged.

> test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 101:
> 
>> 99: try (InputStream in = inputStreamProvider.input(inBytes);
>> 100: OutputStream out = 
>> outputStreamProvider.output(recorder::set)) {
>> 101: in.transferTo(out);
> 
> The return value of `transferTo()` is not checked.

Done in https://github.com/openjdk/jdk/pull/5209, will rebase on that PR once 
merged.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Work on this draft will be continued as soon as depenency PRs are resolved. 
Please keep this issue open.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open. More commits will follow on the next weeks.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open. More commits will follow on the next weeks.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Laurent Bourgès]

On Mon, 6 Dec 2021 07:07:03 GMT, Markus KARG  wrote:

>> Markus KARG has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Draft: Eliminated duplicate code using lambda expressions
>>  - Draft: Use blocking mode also for target channel
>
> Please keep this PR open. More commits will follow on the next weeks.

Looks promising, keep moving, @mkarg !

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Mon, 6 Dec 2021 07:07:03 GMT, Markus KARG  wrote:

>> Markus KARG has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Draft: Eliminated duplicate code using lambda expressions
>>  - Draft: Use blocking mode also for target channel
>
> Please keep this PR open. More commits will follow on the next weeks.

> Looks promising, keep moving, @mkarg !

Thanks a lot, Laurent! I really appreciate! In fact, most of the work is going 
on in spin-off PRs, and some of it already is merged, as I explained on 
[Twitter](https://twitter.com/mkarg/status/1467064781978931206?s=20) and 
[Youtube](https://youtu.be/9Rjz3zRXoD4). :-)

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Thu, 12 Aug 2021 01:04:29 GMT, Brian Burkhalter  wrote:

>> Markus KARG has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Draft: Eliminated duplicate code using lambda expressions
>>  - Draft: Use blocking mode also for target channel
>
> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 249:
> 
>> 247: }
>> 248: 
>> 249: private static long transfer(ReadableByteChannel src, 
>> WritableByteChannel dst) throws IOException {
> 
> Does this method have a measurable improvement in performance over 
> `InputStream.transferTo()`?

Measured it today, here are the actual results:

Sandbox.transferTo  thrpt   25  0,091 ± 0,011  ops/s -- using byte[] (original 
implementation)
Sandbox.transferTo  thrpt   25  0,095 ± 0,012  ops/s -- using utility temporary 
buffer (my contribution)
Sandbox.transferTo  thrpt   23  0,099 ± 0,012  ops/s -- 
FileChannel.transferTo(FileChannel)


The improvement of this method actually is approx. 4% on my Windows laptop, 
which I would say is worth to do it.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open as I am working on several sub-issues currently.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open. I am still working on it.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open; I will continune work on it soon.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

[Markus KARG]

On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG  wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Draft: Eliminated duplicate code using lambda expressions
>  - Draft: Use blocking mode also for target channel

Please keep this PR open; I will continune work on it soon.

-

PR: https://git.openjdk.java.net/jdk/pull/4263