On Fri, 27 Jun 2025 01:03:30 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> 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... > > Kim Barrett 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 11 additional > commits since the last revision: > > - Merge branch 'master' into direct-buffer-cleaner > - Merge branch 'master' into direct-buffer-cleaner > - Merge branch 'master' into direct-buffer-cleaner > - Merge branch 'master' into direct-buffer-cleaner > - add description of BufferCleaner class > - exception handling in cleaner for backward consistency > - detabify > - move jdk.internal.nio.Cleaner to sun.nio > - copyrights > - remove java.nio use of jdk.internal.ref.Cleaner > - ... and 1 more: https://git.openjdk.org/jdk/compare/45819901...c995d97e Looks pretty good. I have some comments. src/java.base/share/classes/java/nio/Bits.java line 202: > 200: throws InterruptedException > 201: { > 202: JavaLangRefAccess jlra = SharedSecrets.getJavaLangRefAccess(); Is it better to have `jlra` here, as a local, versus where it was as a constant? src/java.base/share/classes/java/nio/BufferCleaner.java line 69: > 67: } > 68: > 69: public void clean() { Could be `@Override` src/java.base/share/classes/java/nio/BufferCleaner.java line 74: > 72: // reference processing by BufferCleaner, clear the > referent so > 73: // reference processing is disabled for this object. > 74: clear(); Reference.clear() does not have any JMM guarantees. However, I've not found anything that might race with accessing the referent. src/java.base/share/classes/java/nio/BufferCleaner.java line 88: > 86: } > 87: > 88: // Cribbed from jdk.internal.ref.CleanerImpl. Maybe sometime later this can be shared instead of copied. src/java.base/share/classes/java/nio/BufferCleaner.java line 243: > 241: return; > 242: } > 243: cleaningThread = new CleaningThread(); I think double-checked locking could work well here. ------------- Changes requested by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25289#pullrequestreview-2967551706 PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2172634158 PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2172857797 PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2172822882 PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2172864753 PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2172874211
