-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8359936: StableValues can release the underlying function after complete computation #25878
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
base: master
Are you sure you want to change the base?
Changes from all commits
91f998e
f2c0a1f
6557e2f
45bd635
bc07b13
62e948d
f4fb53a
d585f71
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 |
---|---|---|
|
@@ -126,19 +126,32 @@ public boolean isSet() { | |
@Override | ||
public T orElseSet(Supplier<? extends T> supplier) { | ||
Objects.requireNonNull(supplier); | ||
return orElseSet(supplier, null); | ||
} | ||
|
||
// `supplier` can be null if the `underlyingHolder` released it. | ||
@ForceInline | ||
public T orElseSet(Supplier<? extends T> supplier, | ||
UnderlyingHolder<?> underlyingHolder) { | ||
final Object t = wrappedContentsAcquire(); | ||
return (t == null) ? orElseSetSlowPath(supplier) : unwrap(t); | ||
return (t == null) ? orElseSetSlowPath(supplier, underlyingHolder) : unwrap(t); | ||
} | ||
|
||
@DontInline | ||
private T orElseSetSlowPath(Supplier<? extends T> supplier) { | ||
private T orElseSetSlowPath(Supplier<? extends T> supplier, | ||
UnderlyingHolder<?> underlyingHolder) { | ||
preventReentry(); | ||
synchronized (this) { | ||
final Object t = contents; // Plain semantics suffice here | ||
if (t == null) { | ||
final T newValue = supplier.get(); | ||
// The mutex is not reentrant so we know newValue should be returned | ||
wrapAndSet(newValue); | ||
if (underlyingHolder != null) { | ||
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. Under what circumstances can the underlyingHolder be null 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. If we call this method via the public |
||
// Reduce the counter and if it reaches zero, clear the reference | ||
// to the underlying holder. | ||
underlyingHolder.countDown(); | ||
} | ||
return newValue; | ||
} | ||
return unwrap(t); | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||||||
package jdk.internal.lang.stable; | ||||||||||
|
||||||||||
import jdk.internal.misc.Unsafe; | ||||||||||
import jdk.internal.vm.annotation.ForceInline; | ||||||||||
|
||||||||||
import java.util.Objects; | ||||||||||
import java.util.stream.Stream; | ||||||||||
|
||||||||||
import static java.util.stream.Collectors.joining; | ||||||||||
|
||||||||||
/** | ||||||||||
* This class is thread safe. | ||||||||||
* | ||||||||||
* @param <U> the underlying type | ||||||||||
*/ | ||||||||||
public final class UnderlyingHolder<U> { | ||||||||||
|
||||||||||
private static final Unsafe UNSAFE = Unsafe.getUnsafe(); | ||||||||||
|
||||||||||
private static final long COUNTER_OFFSET = | ||||||||||
UNSAFE.objectFieldOffset(UnderlyingHolder.class, "counter"); | ||||||||||
|
||||||||||
// Used reflectively. This field can only transition at most once from being set to a | ||||||||||
// non-null reference to being `null`. Once `null`, it is never read. This allows | ||||||||||
// the field to be non-volatile, which is crucial for getting optimum performance. | ||||||||||
private U underlying; | ||||||||||
|
||||||||||
// Used reflectively | ||||||||||
private int counter; | ||||||||||
|
||||||||||
public UnderlyingHolder(U underlying, int counter) { | ||||||||||
this.underlying = underlying; | ||||||||||
this.counter = counter; | ||||||||||
// Safe publication | ||||||||||
UNSAFE.storeStoreFence(); | ||||||||||
Comment on lines
+33
to
+35
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. This may actually be substitutable by making the initial write to counter a volatile write:
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. True. We could also use piggybacking. |
||||||||||
} | ||||||||||
|
||||||||||
@ForceInline | ||||||||||
public U underlying() { | ||||||||||
return underlying; | ||||||||||
} | ||||||||||
|
||||||||||
// For testing only | ||||||||||
public int counter() { | ||||||||||
return UNSAFE.getIntVolatile(this, COUNTER_OFFSET); | ||||||||||
} | ||||||||||
|
||||||||||
public void countDown() { | ||||||||||
if (UNSAFE.getAndAddInt(this, COUNTER_OFFSET, -1) == 1) { | ||||||||||
// Do not reference the underlying function anymore so it can be collected. | ||||||||||
underlying = null; | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
} |
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.
By avoiding reading from
underlyingHolder
when theStableValue
is already set, this can avoid the overhead of a mutable field read fromunderlyingHolder
: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.
Before this proposal:
After this proposal:
So, it appears to be some gain here.