New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available #8235
Conversation
… plenty available
👋 Welcome back bpb! A progress list of the required criteria for merging this PR into |
Currently for This change both saves off-heap memory and increases throughput. An allocation of 1 MB is only 0.42% the size of the buffer in the JBS issue, 501 x 501 x 501 x 2 (= 251,503,002), so for this case the memory reduction is drastic. Reading throughput is almost doubled and writing throughput improved by about 50%. As measured on macOS, the throughput of the methods mentioned above before the change was:
and that after is:
|
Webrevs
|
The benchmark results are quite unexpected. Would we benefit from reducing the buffer size even further? |
I tested with smaller and smaller buffer sizes until the performance started to be affected which was about 64 KB. I have not changed this value in the patch just yet. |
By the way: FileOutputStream has exactly the same problem with |
Yes, |
Further performance testing was conducted for the case where the native read and write functions used a fixed, stack-allocated buffer of size 8192. The loops were moved up into the Java code of Here are some updated performance measurements: The performance measurements shown are for the following cases:
The horizontal axis represents a variety of lengths from 8192 to 1GB; the vertical axis is throughput (ops/s) on a log 10 scale. The native lines in the charts are for the code proposed to be integrated. As can be seen, the performance of reading is quite similar up to larger lengths. The mainline version presumably starts to suffer the effect of large allocations. The native read loop performs the best throughout, being for lengths 10 MB and above from 50% to 3X faster than the mainline version. The native read loop is about 40% faster than the Java read loop for these larger lengths. Due to the log scale of the charts, the reading performance detail cannot be seen exactly and so is given here for the larger lengths:
The performance of writing is about the same for the Java and Native versions, as it should be since the implementations are the same. Any difference is likely due to measurement noise. The mainline version again suffers for larger lengths. As the native write loop was already present in the mainline code, the principal complexity proposed to be added is the native read loop. Given the improved throughput and vastly reduced native memory allocation this seems to be justified. |
This change would increase throughput at the expense of the additional complexity of a new loop in the native |
The following benchmark results are for commit e6f3b45 with 5 warmup iterations of 5 seconds each, followed by 10 measurement iterations of 10 seconds each on a MacBookPro16,1.
The proposed change is approximately at parity with the current code base for smaller lengths, but has higher throughput for larger lengths, at least for reading. In all cases where the length is greater than or equal to 65536 (64 KiB) or is a multiple of 8192 (8 KiB), less native memory is allocated. |
Using the temporary direct buffer cache to provide intermediate native memory is not a good solution as direct memory may be limited. Hence the patch is reverted to its prior state, modified to move parameter checking up to the Java layer, and simplified to keep
|
@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! |
Moving to draft. |
@bplb This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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 This pull request has been inactive for more than 16 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 |
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.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8235/head:pull/8235
$ git checkout pull/8235
Update a local copy of the PR:
$ git checkout pull/8235
$ git pull https://git.openjdk.org/jdk pull/8235/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8235
View PR using the GUI difftool:
$ git pr show -t 8235
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8235.diff