RFR: 8339191: JFR: Bulk read support for ChunkInputStream

[Erik Gahlin]

On Sat, 10 Aug 2024 21:55:29 GMT, David Schlosnagle <duke at openjdk.org> wrote:

> I would like to contribute an improvement to reading JFR recordings via `InputStream` by implementing `InputStream#read(byte[], int, int)` in `ChunkInputStream` to support bulk reads and avoid many expensive single byte `read()`.
> 
> Testing: `test/jdk/jdk/jfr/api/consumer/TestChunkInputStreamBulkRead.java`, `jdk_jfr`
> 
> While looking through some JFRs recently, I noticed some significant time spent in `java.io.BufferedInputStream#getBufIfOpen()` that traced back to transferring (via `InputStream#transferTo(OutputStream)` JFR profile recording being read via `InputStream` returned by `jdk.jfr.Recording#getStream(Instant , Instant)`. What caught my eye were calls to single byte `jdk.jfr.internal.ChunkInputStream#read()` rather than `read(byte[], int, int)` as seen in the stacktrace from JFR.
> 
> <img width="776" alt="image" src="https://github.com/user-attachments/assets/b92398cd-2761-4600-8cfe-ea1bada6efd2">

src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 99:

> 97:     @Override
> 98:     public int read(byte[] buf, int off, int len) throws IOException {
> 99:         Objects.checkFromIndexSize(off, len, buf.length);

Might want to check if buf is null, i.e. Object.requireNonNulll(buf, "buf"), to fail fast instead of later in stream.read(buf...) when a file has been opened.

src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 116:

> 114:                 advance();
> 115:                 continue;
> 116:             } else {

Using both "continue" and "else" is a bit confusing. I would drop "continue" and perhaps flip the order of the else-statement so the common/expected branch happens first.

src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 128:

> 126:     }
> 127: 
> 128:     private void advance() throws IOException {

When reading the code, it's a bit unclear what the advance() method does, especially since there is already a nextStream() method. I think the code is easier to understand if closeStream() and closeChunk() are used explicitly and only where needed, i.e. no need to call closeStream() when stream == null has been checked.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20534#discussion_r1736147123
PR Review Comment: https://git.openjdk.org/jdk/pull/20534#discussion_r1736140771
PR Review Comment: https://git.openjdk.org/jdk/pull/20534#discussion_r1736134582