RFR: 8304745: Lazily initialize byte[] in java.io.BufferedInputStream [v3]

[Alan Bateman]

On Fri, 24 Mar 2023 06:40:24 GMT, Sergey Tsypanov <stsypanov at openjdk.org> wrote:

>> By default `BufferedInputStream` is constructed with internal buffer with capacity 8192. In some cases this buffer is never used, e.g. when we call `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when `BufferedInputStream` is cascaded.
>
> Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8304745: Protect getBufIfOpen() from async close

I skimmed through the latest version but there are still several issues.  Can you try the patch below instead? This will ensure that read, skip, reset, etc. check buf as before. It also ensures that read1 does the same threshold check as it has always done - your patch changes that in a subtle way and I think we have to be careful with behavior changes to this really old API.

To Eirik's comment about benchmarks. This change is about memory footprint and lazily creating the internal buffer. A good example is the BIS on stdin which has an 8k buffer that is only needed if something reads from System.in. There may be a few more like this. The other case is a large read where BIS will delegate to the wrapped stream when it doesn't have any bytes buffered. It would be useful to get some data on cases where it helps (the BIS wrapping BIS only works when the enclosing BIS has the same or larger buffer than the enclosed BIS).


--- a/src/java.base/share/classes/java/io/BufferedInputStream.java
+++ b/src/java.base/share/classes/java/io/BufferedInputStream.java
@@ -57,6 +57,8 @@ public class BufferedInputStream extends FilterInputStream {
 
     private static final int DEFAULT_BUFFER_SIZE = 8192;
 
+    private static final byte[] EMPTY = new byte[0];
+
     /**
      * As this class is used early during bootstrap, it's motivated to use
      * Unsafe.compareAndSetObject instead of AtomicReferenceFieldUpdater
@@ -70,6 +72,9 @@ public class BufferedInputStream extends FilterInputStream {
     // initialized to null when BufferedInputStream is sub-classed
     private final InternalLock lock;
 
+    // initial buffer size (DEFAULT_BUFFER_SIZE or size specified to constructor)
+    private final int initialSize;
+
     /**
      * The internal buffer array where the data is stored. When necessary,
      * it may be replaced by another array of
@@ -166,16 +171,42 @@ public class BufferedInputStream extends FilterInputStream {
     }
 
     /**
-     * Check to make sure that buffer has not been nulled out due to
-     * close; if not return it;
+     * Returns the internal buffer, optionally allocating it if empty.
+     * @param allocateIfEmpty true to allocate if empty
+     * @throws IOException if the stream is closed (buf is null)
      */
-    private byte[] getBufIfOpen() throws IOException {
+    private byte[] getBufIfOpen(boolean allocateIfEmpty) throws IOException {
         byte[] buffer = buf;
-        if (buffer == null)
+        if (allocateIfEmpty && buffer == EMPTY) {
+            buffer = new byte[initialSize];
+            if (!U.compareAndSetReference(this, BUF_OFFSET, EMPTY, buffer)) {
+                // re-read buf
+                buffer = buf;
+            }
+        }
+        if (buffer == null) {
             throw new IOException("Stream closed");
+        }
         return buffer;
     }
 
+    /**
+     * Returns the internal buffer, allocating it if empty.
+     * @throws IOException if the stream is closed (buf is null)
+     */
+    private byte[] getBufIfOpen() throws IOException {
+        return getBufIfOpen(true);
+    }
+
+    /**
+     * Throws IOException if the stream is closed (buf is null).
+     */
+    private void ensureOpen() throws IOException {
+        if (buf == null) {
+            throw new IOException("Stream closed");
+        }
+    }
+
     /**
      * Creates a {@code BufferedInputStream}
      * and saves its  argument, the input stream
@@ -205,13 +236,15 @@ public class BufferedInputStream extends FilterInputStream {
         if (size <= 0) {
             throw new IllegalArgumentException("Buffer size <= 0");
         }
-        buf = new byte[size];
-
-        // use monitors when BufferedInputStream is sub-classed
+        initialSize = size;
         if (getClass() == BufferedInputStream.class) {
+            // use internal lock and lazily create buffer when not subclassed
             lock = InternalLock.newLockOrNull();
+            buf = EMPTY;
         } else {
+            // use monitors and eagerly create buffer when subclassed
             lock = null;
+            buf = new byte[size];
         }
     }
 
@@ -307,7 +340,8 @@ public class BufferedInputStream extends FilterInputStream {
                if there is no mark/reset activity, do not bother to copy the
                bytes into the local buffer.  In this way buffered streams will
                cascade harmlessly. */
-            if (len >= getBufIfOpen().length && markpos == -1) {
+            int size = Math.max(getBufIfOpen(false).length, initialSize);
+            if (len >= size && markpos == -1) {
                 return getInIfOpen().read(b, off, len);
             }
             fill();
@@ -374,7 +408,7 @@ public class BufferedInputStream extends FilterInputStream {
     }
 
     private int implRead(byte[] b, int off, int len) throws IOException {
-        getBufIfOpen(); // Check for closed stream
+        ensureOpen();
         if ((off | len | (off + len) | (b.length - (off + len))) < 0) {
             throw new IndexOutOfBoundsException();
         } else if (len == 0) {
@@ -421,7 +455,7 @@ public class BufferedInputStream extends FilterInputStream {
     }
 
     private long implSkip(long n) throws IOException {
-        getBufIfOpen(); // Check for closed stream
+        ensureOpen();
         if (n <= 0) {
             return 0;
         }
@@ -544,7 +578,7 @@ public class BufferedInputStream extends FilterInputStream {
     }
 
     private void implReset() throws IOException {
-        getBufIfOpen(); // Cause exception if closed
+        ensureOpen();
         if (markpos < 0)
             throw new IOException("Resetting to invalid mark");
         pos = markpos;

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1482577711