Thanks, Paul.
CallSite:
public class CallSite {
- // The actual payload of this call site:
+ // The actual payload of this call site.
+ // Can be modified using {@link
MethodHandleNatives#setCallSiteTargetNormal} or {@link
MethodHandleNatives#setCallSiteTargetVolatile}.
/*package-private*/
- MethodHandle target; // Note: This field is known to the JVM. Do not
change.
+ final MethodHandle target; // Note: This field is known to the JVM.
Is there any benefit to making target final, even though it's not really
for mutable call sites? (With the recent discussion of "final means
final" it would be nice to not introduce more special case stomping on
final fields if we can avoid it).
CallSite.target is already treated specially: all updates go through
MethodHandleNatives and JIT-compiler treat it as "final" irrespective of
the flags.
My main interest in marking it final is to enforce proper initialization
on JDK side to make it easier to reason about:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/036021.html
CallSite(MethodType targetType, MethodHandle createTargetHook) throws
Throwable {
- this(targetType);
+ this(targetType); // need to initialize target to make CallSite.type()
work in createTargetHook
ConstantCallSite selfCCS = (ConstantCallSite) this;
MethodHandle boundTarget = (MethodHandle)
createTargetHook.invokeWithArguments(selfCCS);
- checkTargetChange(this.target, boundTarget);
- this.target = boundTarget;
+ setTargetNormal(boundTarget); // ConstantCallSite doesn't publish
CallSite.target
+ UNSAFE.storeStoreFence(); // barrier between target and isFrozen updates
}
I wonder if instead of introducing the store store fence here we could
move it into ConstantCallSite?
Sure, if you prefer to see it on ConstantCallSite side, we can move it
there.
By putting it in CallSite near the call site update, I wanted to stress
there's a CallSite.target update happening on partially published
instance.
Best regards,
Vladimir Ivanov
protected ConstantCallSite(MethodType targetType, MethodHandle
createTargetHook) throws Throwable {
- super(targetType, createTargetHook);
+ super(targetType, createTargetHook); // "this" instance leaks into
createTargetHook
+ UNSAFE.storeStoreFence(); // barrier between target and isFrozen
updates<— Add here
isFrozen = true;
+ UNSAFE.storeStoreFence(); // properly publish isFrozen
}
On Nov 19, 2019, at 9:49 AM, Vladimir Ivanov
<vladimir.x.iva...@oracle.com <mailto:vladimir.x.iva...@oracle.com>>
wrote:
Subtle, so I could be misunderstanding something, did you intend to
remove the assignment of isFrozen in the ConstantCallSite constructor?
Oh, good catch. It is my fault: the update should be there. (The
barriers are added just to preserve final field semantics for isFrozen.)
Published the wrong version (with some leftovers from last-minute
failed experiment). Updated in place.
Best regards,
Vladimir Ivanov
On Nov 19, 2019, at 8:53 AM, Vladimir Ivanov
<vladimir.x.iva...@oracle.com <mailto:vladimir.x.iva...@oracle.com>>
wrote:
http://cr.openjdk.java.net/~vlivanov/8234401/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8234401
ConstantCallSite has a ctor which deliberately leaks partially
initialized instance into user code. isFrozen is declared final and
if user code is obstinate enough, it can end up with non-frozen
state embedded into the generated code. It manifests as a
ConstantCallSite instance which is stuck in non-frozen state.
I switched isFrozen from final to @Stable, so non-frozen state is
never constant folded. Put some store-store barriers along the way
to restore final field handling.
I deliberately stopped there (just restoring isFrozen final field
behavior). Without proper synchronization, there's still a
theoretical possibility of transiently observing a call site in
non-frozen state right after ctor is over. But at least there's no
way anymore to accidentally break an instance in such a way it
becomes permanently unusable.
PS: converted CallSite.target to final along the way and made some
other minor refactorings.
Testing: regression test, tier1-2
Best regards,
Vladimir Ivanov
