RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v3]

[John Hendrikx]

On Thu, 5 May 2022 23:39:30 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Modify native multi-byte read-write code used by the `java.io` classes to limit the size of the allocated native buffer thereby decreasing off-heap memory footprint and increasing throughput.
>
> Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - 6478546: Clean up io_util.c
>  - Merge
>  - 6478546: Decrease malloc'ed buffer maximum size to 64 kB
>  - 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available

src/java.base/share/native/libjava/io_util.c line 79:

> 77:           jint off, jint len, jfieldID fid)
> 78: {
> 79:     char stackBuf[STACK_BUF_SIZE];

This was in the original code already, but doesn't this always allocate 8k on the stack, regardless of whether this buffer will be used (if len > 8k or len == 0)?  Wouldn't it be better to allocate this only in the `else` case?

Would apply to the write code as well.

src/java.base/share/native/libjava/io_util.c line 81:

> 79:     char stackBuf[STACK_BUF_SIZE];
> 80:     char *buf = NULL;
> 81:     jint buf_size, read_size;;

minor: double semi colon

src/java.base/share/native/libjava/io_util.c line 129:

> 127:             off += n;
> 128:         } else if (n == -1) {
> 129:             JNU_ThrowIOExceptionWithLastError(env, "Read error");

The original code would have `nread` set to `-1` here, and thus exit with `-1`.  The new code would exit with the last value for `nread` which could be anything.

src/java.base/share/native/libjava/io_util.c line 201:

> 199:         write_size = len < buf_size ? len : buf_size;
> 200:         (*env)->GetByteArrayRegion(env, bytes, off, write_size, (jbyte*)buf);
> 201:         if (!(*env)->ExceptionOccurred(env)) {

Wouldn't this result in an infinite loop if `GetByteArrayRegion` triggered an exception and `len > 0` ?  It would never enter the `if` and `len` is never changed then...

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

PR: https://git.openjdk.java.net/jdk/pull/8235