On Thu, 12 May 2022 07:59:36 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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.
This is intentional. We want to avoid having a malloc + free in every call and
so avoid it for small buffers.
> 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
FIxed in 10f14bb3faef2b55ab59a85016874d12815f3c87.
> 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.
The returned value of `nread` would in this case indicate the number of bytes
actually read so far, which is intentional.
> 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...
Good catch, you are correct. Fixed in 10f14bb3faef2b55ab59a85016874d12815f3c87.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8235
