[14] RFR (S): 8234401: ConstantCallSite may stuck in non-frozen state

[Paul Sandoz]

Much better :-)

I accumulated some more questions while I was looking further.

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(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?

     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
     }

Paul.

> On Nov 19, 2019, at 9:49 AM, Vladimir Ivanov <vladimir.x.ivanov at 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.ivanov at 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