RFR: 8284638: store skip buffers in InputStream Object [v15]

[Alan Bateman]

On Mon, 30 May 2022 05:29:01 GMT, Alan Bateman <alanb at 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