-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8356255: Add Stable Field Updaters to allow efficient lazy field evaluations #25040
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?
Conversation
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Benchmarks:
Which shows the updater has the same performance as imperative code which is good. |
Updated benchmarks:
|
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java
Outdated
Show resolved
Hide resolved
Webrevs
|
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
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.
Starting to look nicely idiomatic. If I saw this I would however want to add a thin wrapper specific for hash codes where the "zero replacement" and generic types are more hidden.
- HashCoder.forIntField(Foo.class, "hash", ...)
- HashCoder.forLongField(...)
I like that there's an annotation on the field now though!
test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java
Outdated
Show resolved
Hide resolved
test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java
Outdated
Show resolved
Hide resolved
The MH+VH form should be much more lightweight: MH and VH are just MemberName references. I wonder if we can advertise the MH+VH version as an indy BSM:
The CallSite would be constant, with |
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.
StableFieldUpdater.checkAndAdapt(…)
also performs an implicit null‑check:
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
private final Baz baz; | ||
|
||
private static final ToIntFunction<LazyFoo> HASH_UPDATER = | ||
StableFieldUpdater.ofInt(LazyFoo.class, "hash", |
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.
Very clean now.
test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
Outdated
Show resolved
Hide resolved
public static CallSite lazyAtMostOnce(MethodHandles.Lookup lookup, | ||
String unused, | ||
VarHandle accessor, | ||
MethodHandle underlying) { |
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.
This is not a valid bootstrap method signature, as that needs a MethodType
for an invokedynamic
or Class<?>
for a dynamic constant:
public static CallSite lazyAtMostOnce(MethodHandles.Lookup lookup, | |
String unused, | |
VarHandle accessor, | |
MethodHandle underlying) { | |
public static CallSite lazyAtMostOnce(MethodHandles.Lookup lookup, | |
String unused, | |
MethodType type, | |
VarHandle accessor, | |
MethodHandle underlying) { |
* {@return a function that lazily sets the field accessible via the provided | ||
* {@code accessor} by invoking the provided {@code underlying} function | ||
* if the field has its default value (e.g. zero). Otherwise, the | ||
* returned function returns the set field value} | ||
* <p> |
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.
The inline form of @return
is not indented elsewhere in the JDK:
* {@return a function that lazily sets the field accessible via the provided | |
* {@code accessor} by invoking the provided {@code underlying} function | |
* if the field has its default value (e.g. zero). Otherwise, the | |
* returned function returns the set field value} | |
* <p> | |
* {@return a function that lazily sets the field accessible via the provided | |
* {@code accessor} by invoking the provided {@code underlying} function | |
* if the field has its default value (e.g. zero). Otherwise, the returned | |
* function returns the set field value} |
* @throws IllegalArgumentException if the provided {@code underlying} function does | ||
* take exactly one parameter of a reference type. |
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.
* @throws IllegalArgumentException if the provided {@code underlying} function does | |
* take exactly one parameter of a reference type. | |
* @throws IllegalArgumentException if the provided {@code underlying} function does | |
* not take the same parameter types as the | |
* {@code accessor} var handle takes coordinate types. |
final List<Class<?>> shapeCoordinateTypes = new ArrayList<>(varHandle.coordinateTypes().size()); | ||
for (Class<?> coordinate: varHandle.coordinateTypes()) { | ||
shapeCoordinateTypes.add(toObjectIfReference(coordinate)); | ||
} | ||
return new Shape(toObjectIfReference(varHandle.varType()), shapeCoordinateTypes); |
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.
This should construct an unmodifiable list:
final List<Class<?>> shapeCoordinateTypes = new ArrayList<>(varHandle.coordinateTypes().size()); | |
for (Class<?> coordinate: varHandle.coordinateTypes()) { | |
shapeCoordinateTypes.add(toObjectIfReference(coordinate)); | |
} | |
return new Shape(toObjectIfReference(varHandle.varType()), shapeCoordinateTypes); | |
final var coordinateTypes = varHandle.coordinateTypes(); | |
final var shapeCoordinateTypes = new Object[coordinateTypes.size()]; | |
for (int i = 0; i < shapeCoordinateTypes.length; i++) { | |
shapeCoordinateTypes[i] = toObjectIfReference(coordinateTypes.get(i)); | |
} | |
return new Shape(toObjectIfReference(varHandle.varType()), | |
SharedSecrets.getJavaUtilCollectionAccess().listFromTrustedArray(shapeCoordinateTypes)); |
// Allow `invokeExact()` of the `apply(Object)` method | ||
final MethodHandle adaptedUnderlying = underlyingType.parameterType(0).equals(Object.class) | ||
|| underlyingType.parameterType(0).isArray() | ||
? underlying | ||
: underlying.asType(underlyingType.changeParameterType(0, Object.class)); | ||
|
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.
This procedure is repeated in StableFieldUpdaterGenerator::handle(…)
:
// Allow `invokeExact()` of the `apply(Object)` method | |
final MethodHandle adaptedUnderlying = underlyingType.parameterType(0).equals(Object.class) | |
|| underlyingType.parameterType(0).isArray() | |
? underlying | |
: underlying.asType(underlyingType.changeParameterType(0, Object.class)); |
v = (Object) accessor.getAcquire(t); | ||
if (v == null) { | ||
try { | ||
v = (int) underlying.invokeExact(t); |
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’t invokeExact
here, as the return type of underlying
isn’t guaranteed to be Object
:
v = (int) underlying.invokeExact(t); | |
v = (Object) underlying.invoke(t); |
var handle = MhUtil.findStatic(LOCAL_LOOKUP, | ||
"atMostOnce", MethodType.methodType(MethodHandle.class, VarHandle.class, MethodHandle.class)); | ||
return new ConstantCallSite(MethodHandles.insertArguments(handle, 0, accessor, underlying)); |
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.
This makes no sense as a bootstrap for a dynamic call site, as that’ll simply be equivalent to a call to atMostOnce
.
I presume thou meant something like the following, which would actually create the stable field updater for use at the call site:
var handle = MhUtil.findStatic(LOCAL_LOOKUP, | |
"atMostOnce", MethodType.methodType(MethodHandle.class, VarHandle.class, MethodHandle.class)); | |
return new ConstantCallSite(MethodHandles.insertArguments(handle, 0, accessor, underlying)); | |
return new ConstantCallSite(atMostOnce(accessor, underlying).asType(type)); |
or by using the createTargetHook
of ConstantCallSite
:
var handle = MhUtil.findStatic(LOCAL_LOOKUP, | |
"atMostOnce", MethodType.methodType(MethodHandle.class, VarHandle.class, MethodHandle.class)); | |
return new ConstantCallSite(MethodHandles.insertArguments(handle, 0, accessor, underlying)); | |
var handle = MhUtil.findStatic(LOCAL_LOOKUP, | |
"atMostOnce", MethodType.methodType(MethodHandle.class, VarHandle.class, MethodHandle.class)); | |
return new ConstantCallSite(type, MethodHandles.dropArguments( | |
MethodHandles.insertArguments(handle, 0, accessor, underlying), | |
0, ConstantCallSite.class)); |
@minborg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
This sketch shows how "Stable Updaters" can be used to create stable computations of
@Stable
fields. Only one updater is needed per class, similar toAtomicIntegerFieldUpdater
.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040
$ git checkout pull/25040
Update a local copy of the PR:
$ git checkout pull/25040
$ git pull https://git.openjdk.org/jdk.git pull/25040/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25040
View PR using the GUI difftool:
$ git pr show -t 25040
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25040.diff
Using Webrev
Link to Webrev Comment