Ask for a patch review: CallSite.target is not volatile

Rémi Forax forax at univ-mlv.fr
Sat Sep 13 07:27:44 PDT 2008


Hi all,
currently CallSite.target is not volatile, so if an invokedynamic bootstrap
method calls setTarget() in one thread, other threads may not see the 
new target.

Proposed changes :
 - make target volatile
 - use unsafe.putOrdered to set the field in a more lightweight way.
 - change alls field visibility from protected to private because :
    1) protected fields appear in the JavaDoc,
        so another implementation of CallSite (like the one currently 
use by the backport)
        is not allowed.
    2) caller, name and type are final so caller(), name() and type()
        can be used instead.
        subclasses can override checkTarget() to do nothing, in that
        case setTarget() is equivalent to change the target field.
        So I see no reason to declare this fields protected.

let me know your remarks,
Rémi

diff --git a/src/share/classes/java/dyn/CallSite.java 
b/src/share/classes/java/dyn/CallSite.java
--- a/src/share/classes/java/dyn/CallSite.java
+++ b/src/share/classes/java/dyn/CallSite.java
@@ -25,6 +25,8 @@
 
 package java.dyn;
 
+import sun.misc.Unsafe;
+
 /**
  * An <code>invokedynamic</code> call site, as reified to the bootstrap 
method.
  * Every instance of a call site corresponds to a distinct instance
@@ -41,10 +43,22 @@ package java.dyn;
  * @author jrose
  */
 public abstract class CallSite {
-    protected MethodHandle target;
-    protected final Object caller;  // usually a class
-    protected final String name;
-    protected final MethodType type;
+    private volatile MethodHandle target;
+    private final Object caller;  // usually a class
+    private final String name;
+    private final MethodType type;
+   
+    private static final Unsafe unsafe = Unsafe.getUnsafe();
+    private static final long targetOffset;
+
+    static {
+      try {
+          targetOffset = unsafe.objectFieldOffset(
+              CallSite.class.getDeclaredField("target"));
+      } catch(NoSuchFieldException e) {
+          throw new AssertionError(e);
+      }
+    }
 
     protected CallSite(Object caller, String name, MethodType type) {
         this.caller = caller;
@@ -69,7 +83,7 @@ public abstract class CallSite {
      */
     public void setTarget(MethodHandle target) {
         checkTarget(target);
-        this.target = target;
+        unsafe.putOrderedObject(this, targetOffset, target);
     }
    
     protected void checkTarget(MethodHandle target) {




More information about the mlvm-dev mailing list