RFR: 8255878: FilterInputStream is missing implementations of Java 9 InputStream methods
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods. ------------- Commit messages: - 8255878: FilterInputStream is missing implementations of Java 9 InputStream methods Changes: https://git.openjdk.java.net/jdk/pull/5367/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5367&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255878 Stats: 41 lines in 1 file changed: 40 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5367/head:pull/5367 PR: https://git.openjdk.java.net/jdk/pull/5367
On Fri, 3 Sep 2021 22:29:19 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods.
A quick and dirty benchmark of `readAllBytes()` and `readNBytes(int)` revealed throughput thrice that of the version without these overrides. The benchmark consisted of calling these methods on a `BufferedInputStream` with a buffer size of 1 MB wrapping a `FileInputStream` opened on a file of size 1 MB. **Before:** Benchmark Mode Cnt Score Error Units ReadNBytes.readAllBytes thrpt 5 2735.935 ± 26.461 ops/s ReadNBytes.readNBytes thrpt 5 2735.949 ± 16.348 ops/s **After:** Benchmark Mode Cnt Score Error Units ReadNBytes.readAllBytes thrpt 5 8245.034 ± 135.084 ops/s ReadNBytes.readNBytes thrpt 5 8320.554 ± 104.707 ops/s ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8255878: Add @implSpec where appropriate ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5367/files - new: https://git.openjdk.java.net/jdk/pull/5367/files/ceed73ac..3064baee Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5367&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5367&range=00-01 Stats: 31 lines in 1 file changed: 13 ins; 3 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/5367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5367/head:pull/5367 PR: https://git.openjdk.java.net/jdk/pull/5367
On Fri, 3 Sep 2021 23:19:22 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
8255878: Add @implSpec where appropriate
This will probably break all existing subclasses that assume that they only need to override the single `int read()` method - because overriding this single method will now no longer be sufficient. Did you conduct a survey of existing subclasses of FilterInputStream (recursively), in the JDK and elsewhere, to evaluate the impact of this change? ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Sat, 4 Sep 2021 00:39:19 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
This may potentially break any existing subclasses that assume that they only need to override the three `int read()` and `int read(byte[], int, it)` and `int skip(int)` methods - because overriding only these three methods will now no longer be sufficient.
Can you explain this a bit further? InputStream has one abstract method so that's the minimum that has to be implemented. It has base implementations of readAllBytes and readNBytes that would be very inefficient if the 3-arg read is not overridden but I wouldn't expect a correctness issue. ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Sat, 4 Sep 2021 07:02:32 GMT, Alan Bateman <alanb@openjdk.org> wrote:
Can you explain this a bit further? InputStream has one abstract method so that's the minimum that has to be implemented. It has base implementations of readAllBytes and readNBytes that would be very inefficient if the 3-arg read is not overridden but I wouldn't expect a correctness issue.
readAllBytes/readNBytes no longer call any variant of `this.read` - so any subclass that implement the two `read` methods to do something more than what `in.read` does might fail in unexpected ways if `readAllBytes` or `readNBytes` are called. I'm especially concerned with subclasses like e.g. `KeepAliveStream` / `MetteredStream` in `sun.net.www.http` ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Mon, 6 Sep 2021 09:41:54 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
readAllBytes/readNBytes no longer call any variant of `this.read` - so any subclass that implement the two `read` methods to do something more than what `in.read` does might fail in unexpected ways if `readAllBytes` or `readNBytes` are called. I'm especially concerned with subclasses like e.g. `KeepAliveStream` / `MetteredStream` in `sun.net.www.http`
Ah yes, I think you are right. In that case JDK-8255878 can be closed as WNF or else FilterInputStream provides implementations of these methods that don't directly delegate. ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Mon, 6 Sep 2021 12:02:12 GMT, Alan Bateman <alanb@openjdk.org> wrote:
Ah yes, I think you are right. In that case JDK-8255878 can be closed as WNF or else FilterInputStream provides implementations of these methods that don't directly delegate.
We could set a boolean in the constructor if `this.getClass() == FilterInputStream.class` and only delegate in that case. On the other hand - FilterInputStream seems to be intended for subclassing, so maybe that optimization should be done in its subclasses instead, when possible. That said - I see that the class level documentation of FilterInputStream says:
The class FilterInputStream itself simply overrides all methods of InputStream with versions that pass all requests to the contained input stream.
which is no longer strictly true - should this sentence be amended to reflect what really happens? ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Fri, 3 Sep 2021 23:19:22 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods.
Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
8255878: Add @implSpec where appropriate
Created [JDK-8273513](https://bugs.openjdk.java.net/browse/JDK-8273513). Closing this ill-conceived PR as being of too little value compared to the risk of subclass breakage. ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
On Fri, 3 Sep 2021 22:29:19 GMT, Brian Burkhalter <bpb@openjdk.org> wrote:
This request proposes to modify `java.io.FilterInputStream` to override `readAllBytes()`, `readNBytes(int)`, `skipNBytes(long)`, and `transferTo(OutputStream)` in order to leverage any performance advantage that the wrapped stream might have over the `java.io.InputStream` implementations of these methods.
This pull request has been closed without being integrated. ------------- PR: https://git.openjdk.java.net/jdk/pull/5367
participants (3)
-
Alan Bateman
-
Brian Burkhalter
-
Daniel Fuchs