On Mon, 30 May 2022 05:29:01 GMT, Alan Bateman <al...@openjdk.org> 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 (sun.net.www.http) >> - InflaterInputStream (java.util.zip) >> - GZIPInputStream (java.util.zip) >> - ZipFileInflaterInputStream in ZipFile (java.util.zip) >> - ZipInputStream (java.util.zip) >> - JarInputStream (java.util.jar) >> >> Only categories 3 & 6 need skip buffers (15 classes). In all other cases, we >> don't need skip buffer and if we will clean all InputStream subclasses it >> gives much more value than the original PR. >> >> As about 3d party InputStream subclasses - let the owner takes care of the >> implementation. > >> 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. > @AlanBateman this need a csr IMO? Changing InputStream.read to be non-abstract would be an API change and so would require a CSR. I think try it first, the main thing is to satisfy yourself that there aren't any compatibility issues, e.g. when InputStream is extended by an abstract class with a non-abstract read method. ------------- PR: https://git.openjdk.java.net/jdk/pull/5872
