Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Sun, 15 Aug 2021 16:51:59 GMT, Roman Kennke wrote: > Having followed both #4263 and this PR from the sidelines, may I ask why for > #4263 much more rigorous testing is asked but not here? E.g. test for NPE, > random-sized files/buffers, random position, return value, comprehensive JMH > tests to show performance, etc? I'm all for high quality standards and good > test coverage, but why are we seemingly measuring with double standards? I don't think your comment is fair. The FIS PR is modest, covering one scenario, it's easy to review. The serious bugs that we pointed out have been addressed and test coverage has been added. The Channels PR is much more ambitious and is trying to cover a matrix of scenarios involving file channel and selectable channels, lots of corner cases and potential issues, and will require a lot of review cycles. None of the scenarios had any test coverage initially. This has been partially addressed but the proposal is still unwieldy, hence the discussion on reducing the scope to something manageable. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Fri, 13 Aug 2021 14:41:54 GMT, Alan Bateman wrote: > > Is this what you have been asking @mkarg in #4263 to do? Optimize > > transferTo() only for FileInputStream? Would it interfere with #4263? > > #4263 is the input stream returned by Channels.newInputStream where the > source may be a FileChannel and the output stream specified to > InputStream::transferTo may be connected to a FileChannel. Having followed both #4263 and this PR from the sidelines, may I ask why for #4263 much more rigorous testing is asked but not here? E.g. test for NPE, random-sized files/buffers, random position, return value, comprehensive JMH tests to show performance, etc? I'm all for high quality standards and good test coverage, but why are we seemingly measuring with double standards? - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v4]
On Fri, 13 Aug 2021 19:20:48 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Remove unneeded check of position and couunt Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v4]
On Fri, 13 Aug 2021 19:20:48 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Remove unneeded check of position and couunt src/java.base/share/classes/java/io/FileInputStream.java line 373: > 371: } > 372: return transferred + super.transferTo(out); > 373: } Good, I think this version is right. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v4]
> Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8272297: Remove unneeded check of position and couunt - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/796248e7..3e575ba2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5097=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5097=02-03 Stats: 8 lines in 1 file changed: 0 ins; 3 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5097/head:pull/5097 PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v3]
On Fri, 13 Aug 2021 14:53:45 GMT, Brian Burkhalter wrote:
>> Please consider this request to add an override
>> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
>> if the parameter is a `FileOutputStream`.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8272297: Improve growing source and partial transfer cases
src/java.base/share/classes/java/io/FileInputStream.java line 367:
> 365: long count = fc.size() - pos;
> 366: if (pos >= 0 && count >= 0) {
> 367: transferred = fc.transferTo(pos, Long.MAX_VALUE,
> fos.getChannel());
This version looks better I think we can drop L365 and 366. The position cannot
be negative and checking that size >= pos isn't useful. In other words, you can
just call transferTo with the position without these checks.
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v3]
> Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8272297: Improve growing source and partial transfer cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/a199fc67..796248e7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5097=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5097=01-02 Stats: 13 lines in 1 file changed: 1 ins; 3 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5097/head:pull/5097 PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Fri, 13 Aug 2021 14:32:41 GMT, Roman Kennke wrote: > Is this what you have been asking @mkarg in #4263 to do? Optimize > transferTo() only for FileInputStream? Would it interfere with #4263? #4263 is the input stream returned by Channels.newInputStream where the source may be a FileChannel and the output stream specified to InputStream::transferTo may be connected to a FileChannel. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test There is no overlap with #4263. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test Is this what you have been asking @mkarg in #4263 to do? Optimize transferTo() only for FileInputStream? Would it interfere with #4263? - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test src/java.base/share/classes/java/io/FileInputStream.java line 377: > 375: } > 376: } > 377: return super.transferTo(out); I think there is also another bug here for the case that transferTo does a partial transfer, it should be: return transferred + super.transferTo(out)); - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote:
>> Please consider this request to add an override
>> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
>> if the parameter is a `FileOutputStream`.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8272297: Set source position after FC.transferTo(); add test
src/java.base/share/classes/java/io/FileInputStream.java line 364:
> 362: FileChannel fci = getChannel();
> 363: long pos = fci.position();
> 364: long count = fci.size() - pos;
It might be better to just specify the count as Long.MAX_VALUE and test the
size after the transfer. This would be a bit more robust in the scenario that
the file grows and would avoid fallback when the file is truncated, e.g.
FileChannel fc = getChannel();
long pos = fc.position();
long transferred = fc.transferTo(pos, Long.MAX_VALUE, out.getChannel());
long newPos = pos + transferred;
fc.position(newPos);
if (newPos >= fc.size()) {
return transferred;
}
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test Without the source change the test fails, but passes with the change in place. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
> Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8272297: Set source position after FC.transferTo(); add test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/54f14550..a199fc67 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5097=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5097=00-01 Stats: 133 lines in 2 files changed: 132 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5097/head:pull/5097 PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 11:30:16 GMT, Alan Bateman wrote:
>> Please consider this request to add an override
>> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
>> if the parameter is a `FileOutputStream`.
>
> src/java.base/share/classes/java/io/FileInputStream.java line 371:
>
>> 369: // not, then use the superclass method to copy those
>> remaining
>> 370: if (fci.transferTo(pos, count, fco) == count) {
>> 371: return count;
>
> Are you sure this is right? FileChannel transferTo doesn't modify the source
> channel's position so I assume the method above will finish with the
> InputStream at its initial position rather than EOF. How are we on test
> coverage for this case?
You're right, that is stupid. I don't think it affected the benchmark, but
testing likely needs to be revisited.
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 01:16:04 GMT, Brian Burkhalter wrote:
> Please consider this request to add an override
> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
> if the parameter is a `FileOutputStream`.
Changes requested by alanb (Reviewer).
src/java.base/share/classes/java/io/FileInputStream.java line 371:
> 369: // not, then use the superclass method to copy those
> remaining
> 370: if (fci.transferTo(pos, count, fco) == count) {
> 371: return count;
Are you sure this is right? FileChannel transferTo doesn't modify the source
channel's position so I assume the method above will finish with the
InputStream at its initial position rather than EOF. How are we on test
coverage for this case?
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 01:16:04 GMT, Brian Burkhalter wrote: > Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Invoking `transferTo()` on a `FileInputStream` will use the superclass method which copies using looping over buffers of a fixed size. If the parameter is a `FileOutputStream`, the copy is greatly accelerated by using the `java.nio.channels.FileChannel.transferTo()` method with the `FileChannel`s of the source and destination streams. Performance measurements were made for streams of length 10, 100, 1, and 10 bytes. Throughput improvement was observed for all cases on the three usual platforms, with the largest of nearly 5X being seen on Linux because in this case the sendfile() system call is used underneath. In no case did throughput decrease. - PR: https://git.openjdk.java.net/jdk/pull/5097
