jdk
jdk copied to clipboard
openjdk
8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner
This change makes java.nio no longer use jdk.internal.ref.Cleaner to manage native memory for Direct-X-Buffers. Instead it uses bespoke PhantomReferences and a dedicated ReferenceQueue. This differs from PR 22165, which proposed to use java.lang.ref.Cleaner.
This change is algorithmically similar to the two previous versions: JDK-6857566 and JDK-8156500 (current mainline). The critical function is Bits::reserveMemory(). For both of those versions and this change, a thread calls that function and tries to reserve some space. If it fails, then it keeps trying until all cleaners deactivated (cleared) by prior GCs have been cleaned. If reservation still fails, then it invokes the GC to try to deactivate more cleaners for cleaning. After that GC it keeps trying the reservation and waiting for cleaning, with sleeps to avoid a spin loop, eventually either succeeding or giving up and throwing OOME.
Retaining that algorithmic approach is one of the goals of this change, since it has been successfully in use since JDK 9 (and was originally developed and extensively tested in JDK 8).
The key to this approach is having a way to determine that deactivated cleaners have been cleaned. JDK-6857566 accomplished this by having waiting threads help the reference processor until there was no available work. JDK-8156500 waits for the reference processor to quiesce, relying on its immediate processing of cleaners. java.lang.ref.Cleaner doesn't provide a way to do this, which is why this change rolls its own Cleaner-like mechanism from the underlying primitives. Like JDK-6857566, this change has waiting threads help with cleaning references. This was a potentially undesirable feature of JDK-6857566, as arbitrary allocating threads were invoking arbitrary cleaners. (Though by the time of JDK-6857566 the cleaners were only used by DBB, and became internal-only somewhere around that time as well.) That's not a concern here, as the cleaners involved are only from DBB, and we know what they look like.
As noted in the discussion of JDK-6857566, it's good to have DBB cleaning being done off the reference processing thread, as it may be expensive and slow down enqueuing other pending references. JDK-6857566 only did some of that, and JDK-8156500 lost that feature. This change moves all of the DBB cleaning off of the reference processing thread. (So does PR 22165.)
Neither JDK-6857566 nor this change are completely precise. For both, a thread may find there is no available work while other threads have work in progress. Making this change more precise seems to cost complexity and performance. JDK-8156500 is precise in this respect, so we're losing that. But this imprecision wasn't known to cause problems for JDK-6857566, and there hasn't been any evidence of problems with this change either.
During the development of JDK-6857566 it was noticed that parallel cleaning didn't seem to have much (if any) performance benefit. That seems to be true for this change as well.
PR 22165 uses java.lang.ref.Cleaner to manage cleaning. That class doesn't provide a good way to detect progress toward or completion of cleaning of deactivated cleaners from prior GCs. So PR 22165 uses a somewhat clumsy and unreliable mechanism (the canaries) to try to do that. A proposal for such functionality was discussed (in PR 22165) but deemed (probably rightly so) too intrusive. An unpublished alternative was less intrusive, but still might raise questions. The change being proposed here avoids changing or using that class, and performs at least as well.
Another issue with PR 22165 is that if we are indeed out of memory and on our way to OOME, each allocating thread may come up against the slow path lock in Bits::reserveMemory, and in turn perform 9 full GCs and then OOME. That seems kind of pathological. For JDK-6857566, JDK-8156500, and this change, an allocating thread only performs 1 full GC before OOME.
One issue with this change is that it incorporates a near-copy of the CleanableList class from java.lang.ref.Cleaner. Possible future work would merge the two into a common utility. There's another potential client for this: java.desktop/share/classes/sun/java2d/Disposer.java. I tried using a hashtable for this change (as with Disposer), but the CleanableList performed significantly better.
A well-known issue with all of these approaches is -XX:+DisableExplicitGC. If used, then the GCs to request reference processing don't happen. That will likely lead to OOME, though the sleeps might provide an opportunity for automatic GCs to occur, maybe sometimes dodging OOME that way.
https://mail.openjdk.org/pipermail/core-libs-dev/2013-October/021547.html Thread for discussion of development of JDK-6857566
Testing: mach5 tier1-6
Many runs of new tests micro/org/openjdk/bench/java/nio/DirectByteBuffer{GC,Churn} (thanks for those @shipilev), and jdk/java/nio/Buffer/DirectByteBufferAlloc for various versions of this change.
The test java/nio/Buffer/DirectByteBufferAlloc.java can be run explicitly as a
benchmark. But the arguments suggested in that file cause the measurements to
be dominated by full GC times, swamping any other differences. Increasing the
value of XX:MaxDirectMemorySize from 128m to 1024m provides a more useful
comparison.
Result of running that test with -XX:MaxDirectMemorySize=1024m, with other
options as suggested in the file, and comparing the periodic per thread
ms/allocation outputs, produces results like this:
| this change | PR 22165 | baseline | |
|---|---|---|---|
| avg | 0.76165 | 1.00368 | 1.02719 |
| stddev | 0.12396 | 0.18361 | 0.27210 |
| min | 0.54 | 0.6 | 0.59 |
| max | 1.54 | 2.13 | 2.39 |
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Issue
- JDK-8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25289/head:pull/25289
$ git checkout pull/25289
Update a local copy of the PR:
$ git checkout pull/25289
$ git pull https://git.openjdk.org/jdk.git pull/25289/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25289
View PR using the GUI difftool:
$ git pr show -t 25289
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25289.diff
Using Webrev
:wave: Welcome back kbarrett! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@kimbarrett This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner
Reviewed-by: rriggs, bchristi
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 81 new commits pushed to the master branch:
- 854de8c9c6a1d851c1788e5f2250fe0928c51ca4: 8336147: Clarify CDS documentation about static vs dynamic archive
- 16af473397a7b3a6e6e33dd684d0d511168b989b: 8361115: javax/swing/JComboBox/bug4276920.java unnecessarily throws Error instead of RuntimeException
- da0a51ce97453a47b2c7d11e5206774232309e69: 8357601: Checked version of JNI Release
ArrayElements needs to filter out known wrapped arrays - ... and 78 more: https://git.openjdk.org/jdk/compare/8ea544c33fc502208577249fb83544f8d876bc17...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@kimbarrett The following labels will be automatically applied to this pull request:
core-libsnio
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.
Webrevs
- 06: Full - Incremental (e019f088)
- 05: Full - Incremental (c995d97e)
- 04: Full - Incremental (b59a2a9c)
- 03: Full - Incremental (13bf6c2d)
- 02: Full - Incremental (be3312cb)
- 01: Full - Incremental (45d0b1ef)
- 00: Full (3ebf665c)
@AlanBateman The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).
BTW, I'm fine with a suggestion to wait until JDK 26 before integrating this.
It's been a while since anyone other than me did or said anything here. Any takers? @AlanBateman @RogerRiggs @shipilev ?
Going to push as commit 21f2e9a71c31320a8b1248e3970a82b871c63c2b.
Since your change was applied there have been 81 commits pushed to the master branch:
- 854de8c9c6a1d851c1788e5f2250fe0928c51ca4: 8336147: Clarify CDS documentation about static vs dynamic archive
- 16af473397a7b3a6e6e33dd684d0d511168b989b: 8361115: javax/swing/JComboBox/bug4276920.java unnecessarily throws Error instead of RuntimeException
- da0a51ce97453a47b2c7d11e5206774232309e69: 8357601: Checked version of JNI Release
ArrayElements needs to filter out known wrapped arrays - ... and 78 more: https://git.openjdk.org/jdk/compare/8ea544c33fc502208577249fb83544f8d876bc17...master
Your commit was automatically rebased without conflicts.
@kimbarrett Pushed as commit 21f2e9a71c31320a8b1248e3970a82b871c63c2b.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.