RFR: 8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner [v6]

[Brent Christian]

On Fri, 27 Jun 2025 01:03:30 GMT, Kim Barrett <kbarrett at 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