On Tue, 24 May 2022 20:40:51 GMT, XenoAmess <d...@openjdk.java.net> 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) - 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/5872
