Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sun, 17 Dec 2023 09:06:58 GMT, Alan Bateman wrote: >> Sergey Tsypanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8320971: Revert irrelevant changes > > For the method description, I think you can expand it to: > > - does not retain a reference to the byte[] > - does not leak a reference to the byte[] to non-trusted classes > - does not modify the contents of the byte[] > - its 3-arg write does not read the contents outside of the offset/length > bounds > > In passing, this class uses "out" for the target OutputStream in the other > methods, you can keep that consistent. Also I assume the `@see` are left over > from a previous iteration. @AlanBateman Kindly requesting your review. :-) - PR Comment: https://git.openjdk.org/jdk/pull/16879#issuecomment-1872584495
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Tue, 19 Dec 2023 10:14:44 GMT, Sergey Tsypanov wrote: > Shouldn't we keep at least the method for the classes checked in > `BIS.isTrusted()` You can keep it there or inline it. As Alan noted, the main thing is not to add something like a new package. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1431779354
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 17:51:30 GMT, Markus KARG wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8320971: Revert irrelevant changes
>
> test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 85:
>
>> 83: }
>> 84:
>> 85: private static void byteArrayOutputStream(BufferedInputStream bis,
>> byte[] buf) throws IOException {
>
> As we agreed that this PR shall only cover BIS, we can remove the additional
> classes' tests again.
Shouldn't we keep at least the method for the classes checked in
`BIS.isTrusted()` (`ByteArrayOutputStream`, `FileOutputStream`,
`PipedOutputStream`)?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1431195222
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sun, 17 Dec 2023 07:49:16 GMT, Alan Bateman wrote: > > Let's follow Alan's proposal and simply inline this. > > I didn't suggest inlining this. I just asked not to add a new package > com.sun.io with a single method class. Also as the concerns with BAIS and BIS > are not the same (they do overlap but they are not the same), I think it's > best to keep the changes as local as possible. It can be made more widely > useful in the future but great care must be taken. Well, effectively inlining is the *ultimate* locality. ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1429166857
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Thu, 14 Dec 2023 08:47:03 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8320971: Revert irrelevant changes For the method description, I think you can expand it to: - does not retain a reference to the byte[] - does not leak a reference to the byte[] to non-trusted classes - does not modify the contents of the byte[] - its 3-arg write does not read the contents outside of the offset/length bounds In passing, this class uses "out" for the target OutputStream in the other methods, you can keep that consistent. Also I assume the `@see` are left over from a previous iteration. - PR Comment: https://git.openjdk.org/jdk/pull/16879#issuecomment-1859080164
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 17:50:07 GMT, Markus KARG wrote: > Let's follow Alan's proposal and simply inline this. I didn't suggest inlining this. I just asked not to add a new package com.sun.io with a single method class. Also as the concerns with BAIS and BIS are not the same (they do overlap but they are not the same), I think it's best to keep the changes as local as possible. It can be made more widely useful in the future but great care must be taken. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1429051646
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 17:13:17 GMT, Chen Liang wrote: >>> If we drop the method for now I have to write it later again, I guess >> >> Maybe, but it's archived so it's easy enough. > > If this is only used after Module system is ready, we can use something like: > https://github.com/openjdk/jdk/blob/34351b7a7950a3b563748f40f2619374f62f9b16/src/java.base/share/classes/java/nio/file/Files.java#L3352 > > > if (out.getClass().getModule() == Object.class.getModule()) I propose we simply go with the original solution, so we stay sync with BAIS. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428872747
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Thu, 14 Dec 2023 08:47:03 GMT, Sergey Tsypanov wrote:
>> It looks like we can skip copying of `byte[]` in
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in
>> `java.io`.
>>
>> See comment by @vlsi in
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8320971: Revert irrelevant changes
I know it is a bit disappointing to undo the nice changes you did just days
ago, but to get this PR merged quickly it is best to follow Alan's proposal.
src/java.base/share/classes/java/io/BufferedInputStream.java line 677:
> 675: * @see java.io.BufferedInputStream#implTransferTo(OutputStream)
> 676: */
> 677: private static boolean isTrusted(OutputStream os) {
Let's follow Alan's proposal and simply inline this. The solution is in sync
with BAIS then, and we can merge it quickly.
test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 85:
> 83: }
> 84:
> 85: private static void byteArrayOutputStream(BufferedInputStream bis,
> byte[] buf) throws IOException {
As we agreed that this PR shall only cover BIS, we can remove the additional
classes' tests again.
-
Changes requested by mk...@github.com (no known OpenJDK username).
PR Review: https://git.openjdk.org/jdk/pull/16879#pullrequestreview-1785245773
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428872958
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428873231
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 15:55:48 GMT, Brian Goetz wrote: >> @mkarg , thank you for the review, I replied in JIRA to avoid mixing >> comments across different issues: >> https://bugs.openjdk.org/browse/JDK-8321271?focusedId=14634794=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14634794 > > While frozen arrays are on our radar, this is a significant lift, so unlikely > to be coming all that soon. I propose to separate this PR from Vladimir's proposal, so we can merge *this* PR first, and if we later see that Vladimir's changes are accepted, we can merge them *ontop*. Agreed everybody? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428872271
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 00:11:05 GMT, Brian Burkhalter wrote: >> If we drop the method for now I have to write it later again, I guess > >> If we drop the method for now I have to write it later again, I guess > > Maybe, but it's archived so it's easy enough. If this is only used after Module system is ready, we can use something like: https://github.com/openjdk/jdk/blob/34351b7a7950a3b563748f40f2619374f62f9b16/src/java.base/share/classes/java/nio/file/Files.java#L3352 if (out.getClass().getModule() == Object.class.getModule()) - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428864626
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 15:40:50 GMT, Vladimir Sitnikov wrote: >> @vlsi This is a very interesting solution 朗, but it opens a number of >> questions! 樂 As a start, here are mine: >> * You propose a new public method (`OutputStream.write(ByteBuffer)`) as part >> of your solution. We need to discuss whether this is worth the effort to >> change all implementations (and to perceive acceptable performance, all >> custom authors need to optimize their custom classes, too). We also need to >> discuss whether we like the design choice that de facto the public API (not >> just the implementation) of the legacy IO domain (`OutputStream`) is now >> linked to the NIO domain (`ByteBuffer`) (which, IMHO, feels some kind of >> scary beyond `ChannelOutputStream`). >> * Just thinking loudly: I wonder if we could simplify the solution, or if we >> could turn parts of it into some generic `byte[] readOnly(byte[])` utility. >> * I would like to know how much faster the solution with `ByteBuffer` is >> compared to `Arrays.copyOfRange()`: Is it really so huge that it is worth >> the additional trouble? >> * I would like to know how much slower the solution with `ByteBuffer` is >> compared to *skipping* `Arrays.copyOfRange()` for trusted cases (as you >> removed the skipping). >> * I would like to know the performance of custom streams, because your >> default implementation is filling a temporary byte array with a >> Java-implemented byte-by-byte loop, which IMHO would be rather painful >> compared to hardware-supported `copyOrRange()`, and it does that even in >> case of *trusted* targets. >> * @briangoetz Wouldn't we rather teach the Java language some day to provide >> native read-only arrays? That would be much faster, much better to read and >> implies less complex code for the end user. > > @mkarg , thank you for the review, I replied in JIRA to avoid mixing comments > across different issues: > https://bugs.openjdk.org/browse/JDK-8321271?focusedId=14634794=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14634794 While frozen arrays are on our radar, this is a significant lift, so unlikely to be coming all that soon. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428846145
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 14:52:42 GMT, Markus KARG wrote: >> I've created a draft change for `OutputStream#write(ByteBuffer)`, and I have >> benchmarked several cases including >> `ByteArrayInputStream.transferTo(BufferedOutputStream(ByteArrayOutputStream))`, >> `ByteArrayInputStream.transferTo(DataOutputStream(FileOutputStream))`. >> >> Benchmarks show there's improvement in both allocation rate and latency. >> >> Read-only `ByteBuffers` can address poisoning concerns as non-jdk code can't >> peek into read-only arrays. >> >> The `write(ByteBuffer)` approach supports cases like `CheckedOutputStream`, >> `DataOutputStream` which would be hard or impossible to optimize when >> passing naked byte arrays. >> >> Here are the results: >> https://gist.github.com/vlsi/7f4411515a4f2dbb0925fffde92ccb1d >> Here is the diff: >> https://github.com/vlsi/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...write_bytebuffer >> >> @mkarg , @bplb , @AlanBateman , could you please review >> `OutputStream.write(ByteBuffer)` approach? > > @vlsi This is a very interesting solution 朗, but it opens a number of > questions! 樂 As a start, here are mine: > * You propose a new public method (`OutputStream.write(ByteBuffer)`) as part > of your solution. We need to discuss whether this is worth the effort to > change all implementations (and to perceive acceptable performance, all > custom authors need to optimize their custom classes, too). We also need to > discuss whether we like the design choice that de facto the public API (not > just the implementation) of the legacy IO domain (`OutputStream`) is now > linked to the NIO domain (`ByteBuffer`) (which, IMHO, feels some kind of > scary beyond `ChannelOutputStream`). > * Just thinking loudly: I wonder if we could simplify the solution, or if we > could turn parts of it into some generic `byte[] readOnly(byte[])` utility. > * I would like to know how much faster the solution with `ByteBuffer` is > compared to `Arrays.copyOfRange()`: Is it really so huge that it is worth the > additional trouble? > * I would like to know how much slower the solution with `ByteBuffer` is > compared to *skipping* `Arrays.copyOfRange()` for trusted cases (as you > removed the skipping). > * I would like to know the performance of custom streams, because your > default implementation is filling a temporary byte array with a > Java-implemented byte-by-byte loop, which IMHO would be rather painful > compared to hardware-supported `copyOrRange()`, and it does that even in case > of *trusted* targets. > * @briangoetz Wouldn't we rather teach the Java language some day to provide > native read-only arrays? That would be much faster, much better to read and > implies less complex code for the end user. @mkarg , thank you for the review, I replied in JIRA to avoid mixing comments across different issues: https://bugs.openjdk.org/browse/JDK-8321271?focusedId=14634794=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14634794 - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428842060
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Sat, 16 Dec 2023 07:24:59 GMT, Vladimir Sitnikov wrote: >>>The backing array is not frozen so a read-only view doesn't address the >>>second part of the concern where the target keeps a reference. >> >> The users outside of OpenJDK would not have a reference to `byte[]`. >> Do you mean a reference to `ByteBuffer`? >> What if there was a `ByteBuffer#forbidReads()` method that would permanently >> forbid subsequent reads from the `ByteBuffer`? > > I've created a draft change for `OutputStream#write(ByteBuffer)`, and I have > benchmarked several cases including > `ByteArrayInputStream.transferTo(BufferedOutputStream(ByteArrayOutputStream))`, > `ByteArrayInputStream.transferTo(DataOutputStream(FileOutputStream))`. > > Benchmarks show there's improvement in both allocation rate and latency. > > Read-only `ByteBuffers` can address poisoning concerns as non-jdk code can't > peek into read-only arrays. > > The `write(ByteBuffer)` approach supports cases like `CheckedOutputStream`, > `DataOutputStream` which would be hard or impossible to optimize when passing > naked byte arrays. > > Here are the results: > https://gist.github.com/vlsi/7f4411515a4f2dbb0925fffde92ccb1d > Here is the diff: > https://github.com/vlsi/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...write_bytebuffer > > @mkarg , @bplb , @AlanBateman , could you please review > `OutputStream.write(ByteBuffer)` approach? @vlsi This is a very interesting solution 朗, but it opens a number of questions! 樂 As a start, here are mine: * You propose a new public method (`OutputStream.write(ByteBuffer)`) as part of your solution. We need to discuss whether this is worth the effort to change all implementations (and to perceive acceptable performance, all custom authors need to optimize their custom classes, too). We also need to discuss whether we like the design choice that de facto the public API (not just the implementation) of the legacy IO domain (`OutputStream`) is now linked to the NIO domain (`ByteBuffer`) (which, IMHO, feels some kind of scary beyond `ChannelOutputStream`). * Just thinking loudly: I wonder if we could simplify the solution, or if we could turn parts of it into some generic `byte[] readOnly(byte[])` utility. * I would like to know how much faster the solution with `ByteBuffer` is compared to `Arrays.copyOfRange()`: Is it really so huge that it is worth the additional trouble? * I would like to know how much slower the solution with `ByteBuffer` is compared to *skipping* `Arrays.copyOfRange()` for trusted cases (as you removed the skipping). * I would like to know the performance of custom streams, because your default implementation is filling a temporary byte array with a Java-implemented byte-by-byte loop, which IMHO would be rather painful compared to hardware-supported `copyOrRange()`, and it does that even in case of *trusted* targets. * @briangoetz Wouldn't we rather teach the Java language some day to provide native read-only arrays? That would be much faster, much better to read and implies less complex code for the end user. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428830013
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Mon, 4 Dec 2023 14:05:14 GMT, Vladimir Sitnikov wrote: >>> > it doesn't solve the second part of the concern which is read-only view >>> > on the internal buffer. >>> >>> I think `ByteBuffer.asReadOnlyBuffer()` should resolve the issues regarding >>> naked byte access. At least `ByteBuffer#array()` returns `null` for >>> read-only buffers, and `ByteBuffer#hasArray()` returns `false`. Am I >>> missing something? >> >> The backing array is not frozen so a read-only view doesn't address the >> second part of the concern where the target keeps a reference. > >>The backing array is not frozen so a read-only view doesn't address the >>second part of the concern where the target keeps a reference. > > The users outside of OpenJDK would not have a reference to `byte[]`. > Do you mean a reference to `ByteBuffer`? > What if there was a `ByteBuffer#forbidReads()` method that would permanently > forbid subsequent reads from the `ByteBuffer`? I've created a draft change for `OutputStream#write(ByteBuffer)`, and I have benchmarked several cases including `ByteArrayInputStream.transferTo(BufferedOutputStream(ByteArrayOutputStream))`, `ByteArrayInputStream.transferTo(DataOutputStream(FileOutputStream))`. Benchmarks show there's improvement in both allocation rate and latency. Read-only `ByteBuffers` can address poisoning concerns as non-jdk code can't peek into read-only arrays. The `write(ByteBuffer)` approach supports cases like `CheckedOutputStream`, `DataOutputStream` which would be hard or impossible to optimize when passing naked byte arrays. Here are the results: https://gist.github.com/vlsi/7f4411515a4f2dbb0925fffde92ccb1d Here is the diff: https://github.com/vlsi/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...write_bytebuffer @mkarg , @bplb , @AlanBateman , could you please review `OutputStream.write(ByteBuffer)` approach? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428725435
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Fri, 15 Dec 2023 19:35:18 GMT, Sergey Tsypanov wrote: > If we drop the method for now I have to write it later again, I guess Maybe, but it's archived so it's easy enough. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428601842
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Fri, 15 Dec 2023 17:21:52 GMT, Brian Burkhalter wrote: >> I think this would be doing double job, wouldn't it? > >> I think this would be doing double job, wouldn't it? > > Sorry, I don't understand. If we drop the method for now I have to write it later again, I guess - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428403398
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Fri, 15 Dec 2023 09:25:00 GMT, Sergey Tsypanov wrote: > I think this would be doing double job, wouldn't it? Sorry, I don't understand. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1428236374
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Thu, 14 Dec 2023 23:04:25 GMT, Brian Burkhalter wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8320971: Revert irrelevant changes
>
> src/java.base/share/classes/java/io/BufferedInputStream.java line 646:
>
>> 644: int avail = count - pos;
>> 645: if (avail > 0) {
>> 646: if (isTrusted(out)) {
>
> It might be cleaner for now to drop `isTrusted()` and just do the class check
> explicitly here. That still takes care of the main intent of not passing the
> buffer to an untrustworthy stream. Further cleanup and consolidation can be
> done later.
I think this would be doing double job, wouldn't it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1427749255
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Thu, 14 Dec 2023 08:47:03 GMT, Sergey Tsypanov wrote:
>> It looks like we can skip copying of `byte[]` in
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in
>> `java.io`.
>>
>> See comment by @vlsi in
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8320971: Revert irrelevant changes
src/java.base/share/classes/java/io/BufferedInputStream.java line 646:
> 644: int avail = count - pos;
> 645: if (avail > 0) {
> 646: if (isTrusted(out)) {
It might be cleaner for now to drop `isTrusted()` and just do the class check
explicitly here. That still takes care of the main intent of not passing the
buffer to an untrustworthy stream. Further cleanup and consolidation can be
done later.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1427391530
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
> It looks like we can skip copying of `byte[]` in > `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in > `java.io`. > > See comment by @vlsi in > https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: 8320971: Revert irrelevant changes - Changes: - all: https://git.openjdk.org/jdk/pull/16879/files - new: https://git.openjdk.org/jdk/pull/16879/files/42147ce8..8626e926 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16879=14 - incr: https://webrevs.openjdk.org/?repo=jdk=16879=13-14 Stats: 84 lines in 3 files changed: 22 ins; 60 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879 PR: https://git.openjdk.org/jdk/pull/16879
