-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner #25289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
34fc9c6
2564a28
3ebf665
45d0b1e
bed353a
77e83e7
be3312c
3e90def
13bf6c2
b59a2a9
c995d97
efbb1ee
e019f08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||||
/* | ||||||||
* Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights reserved. | ||||||||
* Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights reserved. | ||||||||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||||||||
* | ||||||||
* This code is free software; you can redistribute it and/or modify it | ||||||||
|
@@ -101,8 +101,12 @@ static boolean unaligned() { | |||||||
// increasing delay before throwing OutOfMemoryError: | ||||||||
// 1, 2, 4, 8, 16, 32, 64, 128, 256 (total 511 ms ~ 0.5 s) | ||||||||
// which means that OOME will be thrown after 0.5 s of trying | ||||||||
private static final long INITIAL_SLEEP = 1; | ||||||||
private static final int MAX_SLEEPS = 9; | ||||||||
|
||||||||
private static final Object RESERVE_SLOWPATH_LOCK = new Object(); | ||||||||
private static int RESERVE_GC_EPOCH = 0; // Never negative. | ||||||||
|
||||||||
// These methods should be called whenever direct memory is allocated or | ||||||||
// freed. They allow the user to control the amount of direct memory | ||||||||
// which a process may access. All sizes are specified in bytes. | ||||||||
|
@@ -118,29 +122,41 @@ static void reserveMemory(long size, long cap) { | |||||||
return; | ||||||||
} | ||||||||
|
||||||||
final JavaLangRefAccess jlra = SharedSecrets.getJavaLangRefAccess(); | ||||||||
// Don't completely discard interruptions. Instead, record them and | ||||||||
// reapply when we're done here (whether successfully or OOME). | ||||||||
boolean interrupted = false; | ||||||||
try { | ||||||||
|
||||||||
// Retry allocation until success or there are no more | ||||||||
// references (including Cleaners that might free direct | ||||||||
// buffer memory) to process and allocation still fails. | ||||||||
boolean refprocActive; | ||||||||
do { | ||||||||
// Keep trying to reserve until either succeed or there is no | ||||||||
// further cleaning available from prior GCs. If the latter then | ||||||||
// GC to hopefully find more cleaning to do. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment could better describe what happens. For instance, only one thread at a time will GC, and each thread only GCs once (assuming I'm reading the code right). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made some comment updates that I hope address this. |
||||||||
for (int cleanedEpoch = -1; true; ) { | ||||||||
synchronized (RESERVE_SLOWPATH_LOCK) { | ||||||||
// Test if cleaning for prior GCs (from here) is complete. | ||||||||
// If so, GC to produce more cleaning work, and increment | ||||||||
// the counter to inform other threads that there may be | ||||||||
// more cleaning work to do. This is done under the lock | ||||||||
// to close a race. We could have multiple threads pass | ||||||||
// the test "simultaneously", resulting in back-to-back | ||||||||
// GCs. For a STW GC the window is small, but for a | ||||||||
// concurrent GC it's quite large. | ||||||||
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); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could also do the following which avoids the branch in
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the use of |
||||||||
System.gc(); | ||||||||
break; | ||||||||
} | ||||||||
cleanedEpoch = RESERVE_GC_EPOCH; | ||||||||
} | ||||||||
try { | ||||||||
refprocActive = jlra.waitForReferenceProcessing(); | ||||||||
if (tryReserveOrClean(size, cap)) { | ||||||||
return; | ||||||||
} | ||||||||
} catch (InterruptedException e) { | ||||||||
// Defer interrupts and keep trying. | ||||||||
interrupted = true; | ||||||||
refprocActive = true; | ||||||||
} | ||||||||
if (tryReserveMemory(size, cap)) { | ||||||||
return; | ||||||||
cleanedEpoch = -1; // Reset when incomplete. | ||||||||
} | ||||||||
} while (refprocActive); | ||||||||
|
||||||||
// trigger VM's Reference processing | ||||||||
System.gc(); | ||||||||
} | ||||||||
|
||||||||
// A retry loop with exponential back-off delays. | ||||||||
// Sometimes it would suffice to give up once reference | ||||||||
|
@@ -151,40 +167,53 @@ static void reserveMemory(long size, long cap) { | |||||||
// DirectBufferAllocTest to (usually) succeed, while | ||||||||
// without it that test likely fails. Since failure here | ||||||||
// ends in OOME, there's no need to hurry. | ||||||||
long sleepTime = 1; | ||||||||
int sleeps = 0; | ||||||||
while (true) { | ||||||||
if (tryReserveMemory(size, cap)) { | ||||||||
return; | ||||||||
} | ||||||||
if (sleeps >= MAX_SLEEPS) { | ||||||||
break; | ||||||||
} | ||||||||
for (int sleeps = 0; true; ) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More typical coding pattern in openjdk code. Here and elsewhere in this PR.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the same thing, and doesn't do what's needed here. Perhaps you meant
I like limiting the scope of the variable. Is that a suggestion or a request to change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, your form does better limit the scope of the loop, and is correct as is; (just looks unusual) |
||||||||
try { | ||||||||
if (!jlra.waitForReferenceProcessing()) { | ||||||||
Thread.sleep(sleepTime); | ||||||||
sleepTime <<= 1; | ||||||||
sleeps++; | ||||||||
if (tryReserveOrClean(size, cap)) { | ||||||||
return; | ||||||||
} else if (sleeps < MAX_SLEEPS) { | ||||||||
Thread.sleep(INITIAL_SLEEP << sleeps); | ||||||||
++sleeps; // Only increment if sleep completed. | ||||||||
} else { | ||||||||
throw new OutOfMemoryError | ||||||||
("Cannot reserve " | ||||||||
+ size + " bytes of direct buffer memory (allocated: " | ||||||||
+ RESERVED_MEMORY.get() + ", limit: " + MAX_MEMORY +")"); | ||||||||
} | ||||||||
} catch (InterruptedException e) { | ||||||||
interrupted = true; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// no luck | ||||||||
throw new OutOfMemoryError | ||||||||
("Cannot reserve " | ||||||||
+ size + " bytes of direct buffer memory (allocated: " | ||||||||
+ RESERVED_MEMORY.get() + ", limit: " + MAX_MEMORY +")"); | ||||||||
|
||||||||
} finally { | ||||||||
// Reapply any deferred interruption. | ||||||||
if (interrupted) { | ||||||||
// don't swallow interrupts | ||||||||
Thread.currentThread().interrupt(); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
// Try to reserve memory, or failing that, try to make progress on | ||||||||
// cleaning. Returns true if successfully reserved memory, false if | ||||||||
// failed and ran out of cleaning work. | ||||||||
private static boolean tryReserveOrClean(long size, long cap) | ||||||||
throws InterruptedException | ||||||||
{ | ||||||||
JavaLangRefAccess jlra = SharedSecrets.getJavaLangRefAccess(); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it better to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it doesn't matter, and this puts it close to where it's used. |
||||||||
boolean progressing = true; | ||||||||
while (true) { | ||||||||
if (tryReserveMemory(size, cap)) { | ||||||||
return true; | ||||||||
} else if (BufferCleaner.tryCleaning()) { | ||||||||
progressing = true; | ||||||||
} else if (!progressing) { | ||||||||
return false; | ||||||||
} else { | ||||||||
progressing = jlra.waitForReferenceProcessing(); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
private static boolean tryReserveMemory(long size, long cap) { | ||||||||
|
||||||||
// -XX:MaxDirectMemorySize limits the total capacity rather than the | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,240 @@ | ||
/* | ||
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. Oracle designates this | ||
* particular file as subject to the "Classpath" exception as provided | ||
* by Oracle in the LICENSE file that accompanied this code. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
package java.nio; | ||
|
||
import java.lang.ref.PhantomReference; | ||
import java.lang.ref.Reference; | ||
import java.lang.ref.ReferenceQueue; | ||
import java.util.Objects; | ||
import jdk.internal.nio.Cleaner; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A class cleaner describing the overall objective (an excerpt from the PR description) would be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assume you meant "class comment". I've added such. |
||
class BufferCleaner { | ||
private static final class PhantomCleaner | ||
extends PhantomReference<Object> | ||
implements Cleaner | ||
{ | ||
private final Runnable action; | ||
// Position in the CleanerList. | ||
CleanerList.Node node; | ||
int index; | ||
|
||
public PhantomCleaner(Object obj, Runnable action) { | ||
super(obj, queue); | ||
this.action = action; | ||
} | ||
|
||
public void clean() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Done. |
||
if (cleanerList.remove(this)) { | ||
// If being cleaned explicitly by application, rather than via | ||
// reference processing by BufferCleaner, clear the referent so | ||
// reference processing is disabled for this object. | ||
clear(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reference.clear() does not have any JMM guarantees. However, I've not found anything that might race with accessing the referent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you are concerned about here. However, the referents for |
||
action.run(); | ||
} | ||
} | ||
} | ||
|
||
// Cribbed from jdk.internal.ref.CleanerImpl. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe sometime later this can be shared instead of copied. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the PR description:
I intend to file a JBS issue for that if this change is approved. |
||
static final class CleanerList { | ||
/** | ||
* Capacity for a single node in the list. | ||
* This balances memory overheads vs locality vs GC walking costs. | ||
*/ | ||
static final int NODE_CAPACITY = 4096; | ||
|
||
/** | ||
* Head node. This is the only node where PhantomCleanabls are | ||
* added to or removed from. This is the only node with variable size, | ||
* all other nodes linked from the head are always at full capacity. | ||
*/ | ||
private Node head; | ||
|
||
/** | ||
* Cached node instance to provide better behavior near NODE_CAPACITY | ||
* threshold: if list size flips around NODE_CAPACITY, it would reuse | ||
* the cached node instead of wasting and re-allocating a new node all | ||
* the time. | ||
*/ | ||
private Node cache; | ||
|
||
public CleanerList() { | ||
this.head = new Node(); | ||
} | ||
|
||
/** | ||
* Insert this PhantomCleaner in the list. | ||
*/ | ||
public synchronized void insert(PhantomCleaner phc) { | ||
if (head.size == NODE_CAPACITY) { | ||
// Head node is full, insert new one. | ||
// If possible, pick a pre-allocated node from cache. | ||
Node newHead; | ||
if (cache != null) { | ||
newHead = cache; | ||
cache = null; | ||
} else { | ||
newHead = new Node(); | ||
} | ||
newHead.next = head; | ||
head = newHead; | ||
} | ||
assert head.size < NODE_CAPACITY; | ||
|
||
// Put the incoming object in head node and record indexes. | ||
final int lastIndex = head.size; | ||
phc.node = head; | ||
phc.index = lastIndex; | ||
head.arr[lastIndex] = phc; | ||
head.size++; | ||
} | ||
|
||
/** | ||
* Remove this PhantomCleaner from the list. | ||
* | ||
* @return true if Cleaner was removed or false if not because | ||
* it had already been removed before | ||
*/ | ||
public synchronized boolean remove(PhantomCleaner phc) { | ||
if (phc.node == null) { | ||
// Not in the list. | ||
return false; | ||
} | ||
assert phc.node.arr[phc.index] == phc; | ||
|
||
// Replace with another element from the head node, as long | ||
// as it is not the same element. This keeps all non-head | ||
// nodes at full capacity. | ||
final int lastIndex = head.size - 1; | ||
assert lastIndex >= 0; | ||
if (head != phc.node || (phc.index != lastIndex)) { | ||
PhantomCleaner mover = head.arr[lastIndex]; | ||
mover.node = phc.node; | ||
mover.index = phc.index; | ||
phc.node.arr[phc.index] = mover; | ||
} | ||
|
||
// Now we can unlink the removed element. | ||
phc.node = null; | ||
|
||
// Remove the last element from the head node. | ||
head.arr[lastIndex] = null; | ||
head.size--; | ||
|
||
// If head node becomes empty after this, and there are | ||
// nodes that follow it, replace the head node with another | ||
// full one. If needed, stash the now free node in cache. | ||
if (head.size == 0 && head.next != null) { | ||
Node newHead = head.next; | ||
if (cache == null) { | ||
cache = head; | ||
cache.next = null; | ||
} | ||
head = newHead; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/** | ||
* Segment node. | ||
*/ | ||
static class Node { | ||
// Array of tracked cleaners, and the amount of elements in it. | ||
final PhantomCleaner[] arr = new PhantomCleaner[NODE_CAPACITY]; | ||
int size; | ||
|
||
// Linked list structure. | ||
Node next; | ||
} | ||
} | ||
|
||
private static final class CleaningThread extends Thread { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to https://bugs.openjdk.org/browse/JDK-8346124, should this be creating an |
||
public CleaningThread() {} | ||
|
||
@Override | ||
public void run() { | ||
while (true) { | ||
try { | ||
Cleaner c = (Cleaner) queue.remove(); | ||
c.clean(); | ||
} catch (InterruptedException e) { | ||
// Ignore InterruptedException in cleaner thread. | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Try to do some cleaning. Takes a cleaner from the queue and executes it. | ||
* | ||
* @return true if a cleaner was found and executed, false if there | ||
* weren't any cleaners in the queue. | ||
*/ | ||
public static boolean tryCleaning() { | ||
Cleaner c = (Cleaner) queue.poll(); | ||
if (c == null) { | ||
return false; | ||
} else { | ||
c.clean(); | ||
return true; | ||
} | ||
} | ||
|
||
private static final CleanerList cleanerList = new CleanerList(); | ||
private static final ReferenceQueue<Object> queue = new ReferenceQueue<Object>(); | ||
private static CleaningThread cleaningThread = null; | ||
|
||
private static void startCleaningThreadIfNeeded() { | ||
synchronized (cleanerList) { | ||
if (cleaningThread != null) { | ||
return; | ||
} | ||
cleaningThread = new CleaningThread(); | ||
Comment on lines
+240
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think double-checked locking could work well here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but why bother? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reduce the thread-safety cost of every But perhaps this isn't that hot a code path - depending on how frequently the app creates (and registers) buffers. |
||
} | ||
cleaningThread.setDaemon(true); | ||
cleaningThread.start(); | ||
} | ||
|
||
private BufferCleaner() {} | ||
|
||
/** | ||
* Construct a new Cleaner for obj, with the associated action. | ||
* | ||
* @param obj object to track. | ||
* @param action cleanup action for obj. | ||
* @return associated cleaner. | ||
* | ||
*/ | ||
public static Cleaner register(Object obj, Runnable action) { | ||
Objects.requireNonNull(obj, "obj"); | ||
Objects.requireNonNull(action, "action"); | ||
startCleaningThreadIfNeeded(); | ||
PhantomCleaner cleaner = new PhantomCleaner(obj, action); | ||
cleanerList.insert(cleaner); | ||
Reference.reachabilityFence(obj); | ||
return cleaner; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment on how this is used? Also, it seems more like a "counter".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some comment updates that I hope address this.