-
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 all 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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -101,8 +101,15 @@ 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(); | ||||||||
|
||||||||
// Token for detecting whether some other thread has done a GC since the | ||||||||
// last time the checking thread went around the retry-with-GC loop. | ||||||||
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 +125,45 @@ 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. Once a thread GCs it | ||||||||
// drops to the later retry with backoff loop. | ||||||||
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 change | ||||||||
// the token 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 a thread were to | ||||||||
// somehow be stuck trying to take the lock while enough | ||||||||
// other threads succeeded for the epoch to wrap, it just | ||||||||
// does an excess GC. | ||||||||
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 +174,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 | ||||||||
|
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.