8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner #25289

kimbarrett · 2025-05-18T11:35:55Z

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

Change must not contain extraneous whitespace
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

Link to Webrev Comment

bridgekeeper · 2025-05-18T11:36:45Z

👋 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.

openjdk · 2025-05-18T11:37:31Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-05-18T11:38:07Z

@kimbarrett The following labels will be automatically applied to this pull request:

core-libs
nio

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.

mlbridge · 2025-05-18T11:41:58Z

Webrevs

AlanBateman · 2025-05-18T12:06:54Z

/reviewers 2 reviewer

openjdk · 2025-05-18T12:07:38Z

@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).

AlanBateman · 2025-05-18T12:11:21Z

src/java.base/share/classes/jdk/internal/nio/Cleaner.java

+ * questions.
+ */
+
+package jdk.internal.nio;


The implementation/internal classes for this area are in sun.nio (for historical reasons). Probably best to keep them together.

Good point. Done.

But MhUtil was added to the newly created jdk.internal.invoke package in #20972 instead of adding it to the pre‑existing sun.invoke package.

kimbarrett · 2025-05-18T20:52:05Z

BTW, I'm fine with a suggestion to wait until JDK 26 before integrating this.

ExE-Boss · 2025-05-19T18:30:45Z

src/java.base/share/classes/java/nio/Bits.java

+                    if (RESERVE_GC_EPOCH == cleanedEpoch) {
+                        // Increment with overflow to 0, so the value can
+                        // never equal the initial/reset cleanedEpoch value.
+                        RESERVE_GC_EPOCH = Integer.max(0, RESERVE_GC_EPOCH + 1);


Could also do the following which avoids the branch in Integer.max:

Suggested change

RESERVE_GC_EPOCH = Integer.max(0, RESERVE_GC_EPOCH + 1);

RESERVE_GC_EPOCH = (RESERVE_GC_EPOCH + 1) & Integer.MAX_VALUE;

I think the use of Integer.max is clearer, any performance difference is insignificant, and the compiler can
make that change as an optimization if appropriate. So I'm not inclined to accept this suggestion.

ExE-Boss · 2025-05-19T18:40:56Z

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template

@@ -197,7 +206,7 @@ class Direct$Type$Buffer$RW$$BO$
 #if[rw]
        super(-1, 0, cap, cap, fd, isSync, segment);
        address = addr;
-        cleaner = Cleaner.create(this, unmapper);
+	cleaner = (unmapper == null) ? null : BufferCleaner.register(this, unmapper);


OpenJDK unfortunately uses the less accessible spaces¹:

Suggested change

cleaner = (unmapper == null) ? null : BufferCleaner.register(this, unmapper);

cleaner = (unmapper == null) ? null : BufferCleaner.register(this, unmapper);

Footnotes

Change useTabs to true by default prettier/prettier#7475

https://alexandersandberg.com/tabs-for-accessibility/

↩

OpenJDK has not considered adoption of tabs-for-accessibility and sticks to using spaces across the codebase.

I didn't notice that a tab had snuck in there. Thanks for spotting.

RogerRiggs

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.

RogerRiggs · 2025-05-19T19:08:31Z

src/java.base/share/classes/java/nio/Bits.java

-                if (sleeps >= MAX_SLEEPS) {
-                    break;
-                }
+            for (int sleeps = 0; true; ) {


More typical coding pattern in openjdk code. Here and elsewhere in this PR.

Suggested change

for (int sleeps = 0; true; ) {

while (true) {

int sleeps = 0;

That's not the same thing, and doesn't do what's needed here. Perhaps you meant

int sleeps = 0; while (true) {

I like limiting the scope of the variable. Is that a suggestion or a request to change?

right, your form does better limit the scope of the loop, and is correct as is; (just looks unusual)

RogerRiggs · 2025-05-19T19:16:14Z

src/java.base/share/classes/java/nio/BufferCleaner.java

+import java.lang.ref.ReferenceQueue;
+import java.util.Objects;
+import sun.nio.Cleaner;
+


A class cleaner describing the overall objective (an excerpt from the PR description) would be useful.

Assume you meant "class comment". I've added such.

RogerRiggs · 2025-05-19T19:24:57Z

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template

+            } catch (Throwable x) {
+                // Long-standing behavior: when deallocation fails, VM exits.
+                if (System.err != null) {
+                    new Error("Cleaner terminated abnormally", x).printStackTrace();


The message would be more useful to identify this as a Buffer Cleaner terminated abnormally.

This code was cribbed from #22165, but your suggestion has led me to
discover that wasn't right. I'm going to need to do some rework of the exception handling around cleaning.

RogerRiggs · 2025-05-19T19:27:27Z

src/java.base/share/classes/sun/nio/Cleaner.java

+/**
+ * {@code Cleaner} represents an object and a cleaning action.
+ */
+public interface Cleaner {


Can this be renamed NIOCleaner or NIOBufClenaer or something to avoid the ambiguity between the other cleaner.

I intentionally (re)used the "Cleaner" name to avoid a bunch of renames that
would increase the size of the change and distract from the meat of it. I
think the name to use might be affected by how the implementation of the set
of cleanup objects might get merged between the new java.nio.BufferCleaner and
java.lang.ref.Cleaner. Perhaps the java.lang.ref.Cleaner.Cleanable interface
should be used throughout? I didn't want to expand this change to include
those kinds of questions.

kimbarrett · 2025-05-21T05:42:34Z

src/java.base/share/classes/java/nio/BufferCleaner.java

+                    if (System.err != null) {
+                        new Error("nio Cleaner terminated abnormally", x).printStackTrace();
+                    }
+                    System.exit(1);


This is the same behavior as jdk.internal.ref.Cleaner, for which this class is
substituting in the new regime for DBB management. PR 22165 (and earlier
versions of this PR) put this in the DBB's Deallocator::run method, but I
think it's both clearer here, and better to leave the Deallocator as it was in
mainline and be more consistent with the mainline code.

kimbarrett added 3 commits May 17, 2025 18:34

add micros from Shipilev

34fc9c6

remove java.nio use of jdk.internal.ref.Cleaner

2564a28

copyrights

3ebf665

openjdk bot added the rfr Pull request is ready for review label May 18, 2025

openjdk bot added nio [email protected] core-libs [email protected] labels May 18, 2025

AlanBateman reviewed May 18, 2025

View reviewed changes

move jdk.internal.nio.Cleaner to sun.nio

45d0b1e

ExE-Boss reviewed May 19, 2025

View reviewed changes

RogerRiggs reviewed May 19, 2025

View reviewed changes

kimbarrett added 3 commits May 19, 2025 20:21

detabify

bed353a

exception handling in cleaner for backward consistency

77e83e7

add description of BufferCleaner class

be3312c

kimbarrett commented May 21, 2025

View reviewed changes

kimbarrett added 2 commits May 27, 2025 09:58

Merge branch 'master' into direct-buffer-cleaner

3e90def

Merge branch 'master' into direct-buffer-cleaner

13bf6c2

	RESERVE_GC_EPOCH = Integer.max(0, RESERVE_GC_EPOCH + 1);
	RESERVE_GC_EPOCH = (RESERVE_GC_EPOCH + 1) & Integer.MAX_VALUE;

	cleaner = (unmapper == null) ? null : BufferCleaner.register(this, unmapper);
	cleaner = (unmapper == null) ? null : BufferCleaner.register(this, unmapper);

8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner #25289

Are you sure you want to change the base?

8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner #25289

Uh oh!

Conversation

kimbarrett commented May 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented May 18, 2025

Uh oh!

openjdk bot commented May 18, 2025

Uh oh!

openjdk bot commented May 18, 2025

Uh oh!

mlbridge bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented May 18, 2025

Uh oh!

openjdk bot commented May 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimbarrett commented May 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimbarrett May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kimbarrett commented May 18, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented May 18, 2025 •

edited

Loading

ExE-Boss May 19, 2025 •

edited

Loading

kimbarrett May 19, 2025 •

edited

Loading