RFR: 8304745: Lazily initialize byte[] in java.io.BufferedInputStream
Chen Liang
liach at openjdk.org
Thu Mar 23 19:17:29 UTC 2023
On Wed, 22 Mar 2023 20:34:08 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.
Also, you should document `getBufIfOpen` to indicate that if a caller simply wants to ensure open, they should call `getInIfOpen` instead.
With the `getClass() == BufferedInputStream.class` check, the impact to subclasses is totally averted, and most BufferedInputStream usages, I believe, do not involve subclasses. And this patch should be acceptable without compatibility concerns.
Marked as reviewed by liach (Author).
This change exposes the `CLOSED` array and modifies the specification for `buf`, both of which are protected and thus public APIs. Having the subclass-accessible `buf` `null` after constructor is executed may significantly affect subclasses that reuse such a buffer.
I mean that additional API exposure is acceptable and backward compatible, but the previous contract to subclasses that the protected `buf` is initialized after superclass constructor execution breaks. Before this change proceeds, we have to check how many subclasses of `BufferedInputStream` access this `buf` in a wide range of libraries and evaluate the impact, which I anticipate to be huge.
A slightly less impactful approach would be sharing a `byte[0]` for a newly initialized state, like in ArrayList, so users anticipating a closed `BufferedInputStream` to have a null `buf` will still work. But users anticipating a usable buf after constructor will still break: for instance, since `getBufIfOpen` is private but the `buf` field it uses is protected, nothing prevents subclasses from copying the same implementation over; and the same implementation will stop working with this new optimization.
src/java.base/share/classes/java/io/BufferedInputStream.java line 78:
> 76: private final InternalLock lock;
> 77:
> 78: /**
These should be reverted, now that the subclasses will see the same behavior as before.
src/java.base/share/classes/java/io/BufferedInputStream.java line 211:
> 209: throw new IllegalArgumentException("Buffer size <= 0");
> 210: }
> 211: buf = new byte[0];
Two recommendations:
1. This `new byte[0]` can be safely shared, so I recommend you to put it in a static final field to avoid reallocation
2. The subclass compatibility can be retained by:
Suggestion:
buf = (getClass() == BufferedInputStream.class) ? new byte[0] : new byte[size];
src/java.base/share/classes/java/io/BufferedInputStream.java line 216:
> 214: throw new IllegalArgumentException("Buffer size <= 0");
> 215: }
> 216: buf = getClass() == BufferedInputStream.class ? EMPTY : new byte[size];
Actually, you can merge this `getClass()` check with the one below for locks.
src/java.base/share/classes/java/io/BufferedInputStream.java line 217:
> 215: }
> 216: this.size = size;
> 217: // use monitors when BufferedInputStream is sub-classed
Maybe update this comment to be like:
Suggestion:
// keep using monitors and initializing buf when BufferedInputStream is sub-classed
// for compatibility
-------------
PR Review: https://git.openjdk.org/jdk/pull/13150#pullrequestreview-1354736052
PR Review: https://git.openjdk.org/jdk/pull/13150#pullrequestreview-1354917871
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1480363127
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1480711123
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146272964
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146268740
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146390661
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146465984
More information about the core-libs-dev
mailing list