On Sun, 18 May 2025 20:55:48 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 incrementally with one additional
> commit since the last revision:
>
> move jdk.internal.nio.Cleaner to sun.nio
The initial PR description is copied into every email. The detail in the PR is
appreciated but can be less intrusive if included in a comment after the
initial description.
src/java.base/share/classes/java/nio/Bits.java line 170:
> 168: // without it that test likely fails. Since failure here
> 169: // ends in OOME, there's no need to hurry.
> 170: for (int sleeps = 0; true; ) {
More typical coding pattern in openjdk code. Here and elsewhere in this PR.
Suggestion:
while (true) {
int sleeps = 0;
src/java.base/share/classes/java/nio/BufferCleaner.java line 33:
> 31: import java.util.Objects;
> 32: import sun.nio.Cleaner;
> 33:
A class cleaner describing the overall objective (an excerpt from the PR
description) would be useful.
src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template line 88:
> 86: // Long-standing behavior: when deallocation fails, VM
> exits.
> 87: if (System.err != null) {
> 88: new Error("Cleaner terminated abnormally",
> x).printStackTrace();
The message would be more useful to identify this as a **Buffer** Cleaner
terminated abnormally.
src/java.base/share/classes/sun/nio/Cleaner.java line 31:
> 29: * {@code Cleaner} represents an object and a cleaning action.
> 30: */
> 31: public interface Cleaner {
Can this be renamed NIOCleaner or NIOBufClenaer or something to avoid the
ambiguity between the other cleaner.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25289#pullrequestreview-2851749648
PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2096331929
PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2096341238
PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2096351666
PR Review Comment: https://git.openjdk.org/jdk/pull/25289#discussion_r2096355156
