Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Mon, 30 May 2022 05:29:01 GMT, Alan Bateman wrote: >> More practically. >> This PR has a noticeable negative effect - it increases the size of >> InputStream objects. Moreover, it increases the size of InputStream >> subclasses which has own skip() implementation and don't need this >> superclass modification. >> >> Let's look into InputStream subclasses. >> I've checked 51 InputStream from classlib. 30 of them either have their own >> skip() implementation or use super.skip() other than from InputStream. This >> PR is definitely harmful to these 30 classes. These 30 classes can be >> divided into the following categories: >> >> 1) Clean non-allocating skip() implementation (22 classes): >> - BufferedInputStream (java.io) >> - ByteArrayInputStream (java.io) >> - FileInputStream (java.io) >> - FilterInputStream (java.io) >> - PeekInputStream in ObjectInputStream (java.io) >> - BlockDataInputStream in ObjectInputStream (java.io) >> - PushbackInputStream (java.io) >> - FastInputStream in Manifest (java.util.jar) >> - ZipFileInputStream in ZipFile (java.util.zip) >> - CipherInputStream (javax.crypto) >> - MeteredStream (sun.net.www) >> - Anonymous in nullInputStream() in InputStream (java.io) >> - DataInputStream (java.io) >> - Anonymous in readTrailer() in GZIPInputStream (java.util.zip) >> - FtpInputStream in FtpURLConnection (sun.net.www.protocol.ftp) >> - JarURLInputStream in JarURLConnection (sun.net.www.protocol.jar) >> - PlainTextInputStream (sun.net.www.content.text) >> - PushbackInputStream (java.io) >> - TelnetInputStream (sun.net) >> - UnclosableInputStream in FileInputStreamPool (sun.security.provider) >> - MeteredStream (sun.net.www) >> - KeepAliveStream (sun.net.www.http) >> >> 2) Partially clean skip() implementation (1 class): >> - ChannelInputStream (sun.nio.ch) >>Note: clean skip impl when using SeekableByteChannel (most usages) >> otherwise super.skip() is used, and it may be easily rewritten using the >> existing internal buffer. >> >> 3) Unavoidable skip buffers (7 classes): >> - PipeInputStream in Process (java.lang) // per skip() invocation buffer >> byte[2048] >> - CheckedInputStream (java.util.zip) // per skip() invocation buffer >> byte[512] >> - InflaterInputStream (java.util.zip)// per skip() invocation buffer >> byte[512] >> - AppInputStream in SSLSocketImpl (sun.security.ssl) // per skip() >> invocation buffer byte[256] >> - DeflaterInputStream (java.util.zip) // cached skip buffer, byte[512], >> allocated on demand >> - ZipInputStream (java.util.zip) // preallocated skip buffer >> byte[512] >> - HttpInputStream in HttpURLConnection (sun.net.www.protocol.http) // >> cached skip buffer, byte[8096], allocated on demand >> >> It would be better to clean all skip implementations for the latter category >> and make it consistent. Now it's totally a mess. All possible strategies >> were implemented. >> >> Now let's consider classes which uses InputStream.skip() implementation (21 >> classes): >> >> 4) skip() is not implemented, when trivial implementation is possible (4 >> classes): >> - EmptyInputStream (sun.net.www.protocol.http) >> - NullInputStream in ProcessBuilder (java.lang) >> - ObjectInputStream (java.io) >> - extObjectInputStream (javax.crypto) >> >> 5) skip() is not implemented, when not-so-trivial implementation is possible >> (9 classes): >> - Anonymous in getInputStream() in NioSocketImpl (sun.nio.ch) >>Notes: temp direct buffer is used for reading, it's possible to implement >> skip over the direct buffer, save allocation + no copying from direct buffer >> to byte[] >> - Anonymous in newInputStream() in Channels (java.nio.channels) >>Notes: temp direct buffer is used for reading, it's possible to implement >> skip over the direct buffer, save allocation + no copying from direct buffer >> to byte[] >> - ChunkedInputStream (sun.net.www.http) >>Notes: skip() maybe implemented with the existing internal buffer >> - DecInputStream in Base64 (java.util) >> - ErrorStream in HttpURLConnection (sun.net.www.protocol.http) >>Notes: skip (but easily can be implemented with internal buffer + >> delegation to tail stream) >> - PipedInputStream (java.io) >>Notes: skip() may be implemented with the existing internal buffer >> - SequenceInputStream (java.io) >>Notes: skip() maybe implemented with delegation >> - SocketInputStream (sun.nio.ch) >>Notes: temp direct buffer is used for reading, it's possible to implement >> skip over the direct buffer, save allocation + no copying from direct buffer >> to byte[] >> - SocketInputStream in Socket (java.net) >>Notes: skip() maybe implemented with delegation >> >> And the last category: >> >> 6) skip() is not implemented, and skip buffer is unavoidable (8 classes): >> - VerifierStream in JarVerifier (java.util.jar) >> - DigestInputStream (java.security) >> - HttpCaptureInputStream
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Mon, 30 May 2022 05:29:01 GMT, Alan Bateman wrote: > However, I think your suggestion to change the no-arg read/write be > non-abstract is interesting as it's always a pain to have to implement that. @AlanBateman this need a csr IMO? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Sun, 29 May 2022 18:15:52 GMT, Sergey Kuksenko wrote: > 5. skip() is not implemented, when not-so-trivial implementation is possible > (9 classes): For the low-level streams (e.g. connected to socket) then it would be common to see them wrapped by buffered streams. So it might not be worth doing anything there. However, I think your suggestion to change the no-arg read/write be non-abstract is interesting as it's always a pain to have to implement that. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Sun, 29 May 2022 18:23:03 GMT, Sergey Kuksenko wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> invert if; refine javadoc. > > Another InputStreams cleaning direction. > > Many InputStream subclasses have read(byte[],...) as a primary > implementation. Somehow they should implement 1-byte reading (read()) via > array reading. All imaginable strategies are used: > - allocate byte[1] per read() invocation > - cache byte[1] preallocated > - cache byte[1], allocation on demand > - share 1-byte read buffer and skip buffer, as preallocating and allocating > on demand. > - etc > > So cleaning and making consistent all 1-byte read implementations - also may > be valuable. @kuksenko good suggestions. I would have a try when have time. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Tue, 24 May 2022 20:40:51 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > invert if; refine javadoc. Another InputStreams cleaning direction. Many InputStream subclasses have read(byte[],...) as a primary implementation. Somehow they should implement 1-byte reading (read()) via array reading. All imaginable strategies are used: - allocate byte[1] per read() invocation - cache byte[1] preallocated - cache byte[1], allocation on demand - share 1-byte read buffer and skip buffer, as preallocating and allocating on demand. - etc So cleaning and making consistent all 1-byte read implementations - also may be valuable. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Tue, 24 May 2022 20:40:51 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > invert if; refine javadoc. More practically. This PR has a noticeable negative effect - it increases the size of InputStream objects. Moreover, it increases the size of InputStream subclasses which has own skip() implementation and don't need this superclass modification. Let's look into InputStream subclasses. I've checked 51 InputStream from classlib. 30 of them either have their own skip() implementation or use super.skip() other than from InputStream. This PR is definitely harmful to these 30 classes. These 30 classes can be divided into the following categories: 1) Clean non-allocating skip() implementation (22 classes): - BufferedInputStream (java.io) - ByteArrayInputStream (java.io) - FileInputStream (java.io) - FilterInputStream (java.io) - PeekInputStream in ObjectInputStream (java.io) - BlockDataInputStream in ObjectInputStream (java.io) - PushbackInputStream (java.io) - FastInputStream in Manifest (java.util.jar) - ZipFileInputStream in ZipFile (java.util.zip) - CipherInputStream (javax.crypto) - MeteredStream (sun.net.www) - Anonymous in nullInputStream() in InputStream (java.io) - DataInputStream (java.io) - Anonymous in readTrailer() in GZIPInputStream (java.util.zip) - FtpInputStream in FtpURLConnection (sun.net.www.protocol.ftp) - JarURLInputStream in JarURLConnection (sun.net.www.protocol.jar) - PlainTextInputStream (sun.net.www.content.text) - PushbackInputStream (java.io) - TelnetInputStream (sun.net) - UnclosableInputStream in FileInputStreamPool (sun.security.provider) - MeteredStream (sun.net.www) - KeepAliveStream (sun.net.www.http) 2) Partially clean skip() implementation (1 class): - ChannelInputStream (sun.nio.ch) Note: clean skip impl when using SeekableByteChannel (most usages) otherwise super.skip() is used, and it may be easily rewritten using the existing internal buffer. 3) Unavoidable skip buffers (7 classes): - PipeInputStream in Process (java.lang) // per skip() invocation buffer byte[2048] - CheckedInputStream (java.util.zip) // per skip() invocation buffer byte[512] - InflaterInputStream (java.util.zip)// per skip() invocation buffer byte[512] - AppInputStream in SSLSocketImpl (sun.security.ssl) // per skip() invocation buffer byte[256] - DeflaterInputStream (java.util.zip) // cached skip buffer, byte[512], allocated on demand - ZipInputStream (java.util.zip) // preallocated skip buffer byte[512] - HttpInputStream in HttpURLConnection (sun.net.www.protocol.http) // cached skip buffer, byte[8096], allocated on demand It would be better to clean all skip implementations for the latter category and make it consistent. Now it's totally a mess. All possible strategies were implemented. Now let's consider classes which uses InputStream.skip() implementation (21 classes): 4) skip() is not implemented, when trivial implementation is possible (4 classes): - EmptyInputStream (sun.net.www.protocol.http) - NullInputStream in ProcessBuilder (java.lang) - ObjectInputStream (java.io) - extObjectInputStream (javax.crypto) 5) skip() is not implemented, when not-so-trivial implementation is possible (9 classes): - Anonymous in getInputStream() in NioSocketImpl (sun.nio.ch) Notes: temp direct buffer is used for reading, it's possible to implement skip over the direct buffer, save allocation + no copying from direct buffer to byte[] - Anonymous in newInputStream() in Channels (java.nio.channels) Notes: temp direct buffer is used for reading, it's possible to implement skip over the direct buffer, save allocation + no copying from direct buffer to byte[] - ChunkedInputStream (sun.net.www.http) Notes: skip() maybe implemented with the existing internal buffer - DecInputStream in Base64 (java.util) - ErrorStream in HttpURLConnection (sun.net.www.protocol.http) Notes: skip (but easily can be implemented with internal buffer + delegation to tail stream) - PipedInputStream (java.io) Notes: skip() may be implemented with the existing internal buffer - SequenceInputStream (java.io) Notes: skip() maybe implemented with delegation - SocketInputStream (sun.nio.ch) Notes: temp direct buffer is used for reading, it's possible to implement skip over the direct buffer, save allocation + no copying from direct buffer to byte[] - SocketInputStream in Socket (java.net) Notes: skip() maybe implemented with delegation And the last category: 6) skip() is not implemented, and skip buffer is unavoidable (8 classes): - VerifierStream in JarVerifier (java.util.jar) - DigestInputStream (java.security) - HttpCaptureInputStream (sun.net.www.http) - InflaterInputStream (java.util.zip) -
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Thu, 26 May 2022 15:43:57 GMT, XenoAmess wrote: > > Is there any practical scenario where the current code (skip buffer > > allocation on each invocation) creates issues? > > @kuksenko Not found any yet :) In that case, what is the value of this PR? Do we need a code change for the sake of code change? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Wed, 25 May 2022 23:23:13 GMT, Sergey Kuksenko wrote: > Is there any practical scenario where the current code (skip buffer > allocation on each invocation) creates issues? @kuksenko Not found any yet :) - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Tue, 24 May 2022 20:40:51 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > invert if; refine javadoc. Is there any practical scenario where the current code (skip buffer allocation on each invocation) creates issues? Most leaf InputStreams have their own skip implementation. And the same question for Reader class. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: invert if; refine javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/d082eae4..cdc4e579 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5872=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5872=13-14 Stats: 8 lines in 1 file changed: 5 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
