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

[Markus KARG]

On Thu, 3 Jun 2021 17:29:14 GMT, Markus KARG <github.com+1701815+mkarg at openjdk.org> 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