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

[John Rose]

On Sep 13, 2008, at 7:27 AM, Rémi Forax wrote:

> 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.

Hmm...  The EDR is ambiguous about this, and needs to be tightened  
up.  It does say that the bulk invalidation API is intended to  
provide a global interlock, and gives some freedom to the propagation  
of setTarget effects.  What the EDR says about "next call" refers to  
a global ordering that doesn't really exist in the JVM apart from  
synchronization.

Native JVM implementations don't require target to be explicitly  
volatile.  In fact, individual threads need the freedom to cache the  
target, e.g., as a loop invariant for a fast-path version of a loop.   
The important thing to optimize here is the speed of *reading* the  
target field.  Writes can be as slow as we want (I use a native  
method call).

All this is independent of whether the code says "volatile" or not on  
the field, but it may be better not to mark it volatile, since that  
seems to make a guarantee about propagation of setTarget effects.

Note that the JVM reads the target field in a specialized way, not  
necessarily through a bytecode.  Also, the writes to the target field  
go through an accessor, so once again, the volatility of the field  
does not necessarily matter.  Except to the backport.  For the  
backport, I guess the volatile bit is needed.  So, yes, that's a good  
change.

Maybe the right thing to do is move target into a mixin-style  
superclass, java.dyn.impl.CS, and have it be volatile in the backport.

This takes me to the second proposed change.

>  - 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.

I want the CallSite to be usable two ways:
1. created by the JVM to reify invokedynamic instructions
2. creatable by Java code to simulate invokedynamic sites interpretively

Therefore, it's important to allow Java code to make its own flavor  
of CallSite.  Hence the protected methods & fields.

This could also be done with a CallSite interface, but it seems  
easier to me (and to other JVM vendors I think) to make it a concrete  
class.

Therefore, let's keep the protected stuff for now, unless we decide  
to make CallSite into an interface.

I want to factor the 292 API into common classes, including the  
public ones, and non-standard implementation-specific classes, one  
version for each kind of JVM and one for the backport.  The  
java.dyn.impl package contains the implementation-specific classes.   
(This name will change to go outside of the java.* hierarchy.)  When  
a public type needs to mix in fields or methods, it does so via  
inheritance from an implementation superclass.  Switching between  
backport and native implementations will therefore require  
adjustments to boot class path or the installed rt.jar.

Why use mixins?  Although such pluggable implementations are more  
often done via delegation or subclassing, the performance  
requirements of method handles requires a more aggressive (less  
flexible) pattern of single-inheritance mixins.

-- John