On Mon, 27 Mar 2023 15:24:05 GMT, Brian Burkhalter <b...@openjdk.org> wrote:
>> 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;
>
>> Can you try the patch below instead? This will ensure that read, skip,
>> reset, etc. check buf as before.
>
> I think that the implementation provided by @AlanBateman looks good.
The BIS changes in this PR looks okay. I can't be Reviewer for that part
because I provided the current changes. @bplb tells me that he will review.
I don't have strong views on the benchmark. The main benefit of the lazy
allocation is, in my view, for cases where a BIS is created and either not
used, or the first usage is long after it is created. We see this with
System.in but it's possible that most BIS usages involve a read soon after the
BIS is created. The other scenario is the subtle optimization to bypass the
internal buffer. This only works in the benchmark because readAllBytes uses a
16k buffer.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1495839430
