6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available

[bplb]

Limit native memory allocation and move write loop from the native layer into Java. This change should make the OOME reported in the issue much less likely.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14981/head:pull/14981
$ git checkout pull/14981

Update a local copy of the PR:
$ git checkout pull/14981
$ git pull https://git.openjdk.org/jdk.git pull/14981/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14981

View PR using the GUI difftool:
$ git pr show -t 14981

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14981.diff

Webrev

Link to Webrev Comment

[bplb]

The cost of native memory allocation appears to degrade the throughput of reads and writes of larger arrays. Above a certain size, allocating and looping over a smaller array was measured to achieve higher throughput.

As Objects::checkFromIndexSize now guards against out of bounds conditions, and obtaining the array length as its third parameter provides a null check, the NULL and bounds checks could be removed from the C code.

[bridgekeeper]

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@bplb The following labels will be automatically applied to this pull request:

• core-libs
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 05: Full - Incremental (408e2df6)
• 04: Full - Incremental (3a016a72)
• 03: Full - Incremental (cdb8455c)
• 02: Full - Incremental (92649751)
• 01: Full - Incremental (69941de4)
• 00: Full (8d7b3478)

[mlbridge]

Mailing list message from Bernd on core-libs-dev:

An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20230724/2f814319/attachment.htm>

[mlbridge]

Mailing list message from Brian Burkhalter on core-libs-dev:

I think the idea is to treat an IOException thrown by any but the first invocation of readBytes() as equivalent to end-of-file such as described for InputStream::read(byte[],int,int)<https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/io/InputStream.html#read(byte%5B%5D,int,int)> with respect to its invocation of InputStream::read()<https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/io/InputStream.html#read()>.

On Jul 24, 2023, at 8:33 AM, Bernd <ecki at zusammenkunft.net<mailto:ecki at zusammenkunft.net>> wrote:

If you return the short buffer (on IOException), does it need to cache the pending exception or can you just rely on the next read hitting that underlying exception again? (Is there an actual guarantee somewhere that the read position is not altered on IOExceptions or that you always get lending bytes before this report? (If so, does the native code do that as well?)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20230724/5df088ff/attachment-0001.htm>

[AlanBateman]

I looked through the latest version (69941de).

I think the main issue with this version is that it changes behavior of the read methods to work like "read fully", I don't think we should do that. To preserve long standing behavior it should attempt a second/subsequent read when the clamped buffer is filled. I think this is close to what you want (not tested, but you'll get the idea I think).

    private int readBytes(byte[] b, final int off, int len) throws IOException {
        Objects.checkFromIndexSize(off, len, b.length);
        int nread = 0;
        int pos = off;
        int remaining = len;
        do {
            int size = Math.min(remaining, 64 * 1024);
            try {
                int n = readBytes0(b, pos, size);
                if (n < 0) {
                    // EOF
                    break;
                }
                nread += n;
                if (n < size) {
                    // buffer not filled
                    break;
                }
                pos += n;
                remaining -= n;
            } catch (IOException ioe) {
                if (nread > 0) {
                    break;
                }
                throw ioe;
            }
        } while (remaining > 0);
        return nread;
    }

The changes in 69941de are also using nested blockers, this is benign, but if this code is change then they can be dropped from the callers.

[bplb]

I think the main issue with this version is that it changes behavior of the read methods to work like "read fully", I don't think we should do that.

To me it looks more like readNBytes.

[bplb]

The changes in 69941de are also using nested blockers, this is benign, but if this code is change then they can be dropped from the callers.

I don't see where this is happening.

[bplb]

The EOF handling in the above should be

                    if (n < 0) {
                        // EOF
                        if (nread == 0)
                            return -1;
                        break;
                    }

or zero will be returned if EOF is encountered on the first read (I made the same mistake in code that was not checked in).

[bplb]

I think this is close to what you want

Modified as suggested in 9264975. The limit is increased to 1.5 Mb where it was in native malloc clamping. This appears to prevent any throughput degradation.

[AlanBateman]

1.5Mb seems high, I think we really need micro that do real file I/O to help tune this.

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bplb]

Still need to derive a valid value for the maximum read/write size by benchmarks which are not just measuring the file system cache.

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@bplb This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.