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

[Rémi Forax]

John Rose a écrit :
> 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.
>   
yop, i have seen that. I'm currently not able to provide
that requirement for the backport.
> 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).
>   
The problem is if a new class is loaded by another thread,
I may want to change the target for any other threads.
If you cache the target, it will not change when a new class is discovered.
i.e the target will change in RAM but not in the local cache.

ConcurrentLinkedQueue<Data> queue=...

thread1:
for(Data data:queue) {
   // invokedynamic call using data
}


thread2:
queue.add(new DataSubClass());
// here the classloader load DataSubClass and
// defeat the optimisation done at
// invokedynamic call site.


> 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. 
not agree, if the accessor is inlined, target can be not reloaded from 
the RAM.
>  Except to the backport.  For the  
> backport, I guess the volatile bit is needed.  So, yes, that's a good  
> change.
>   
Yes.
> 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.
>   
I'am not sure you can avoid the volatile read.
> 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.
>   
You don't need protected field if you have accessors. And currently
those accessors exist.
> 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 not use a static final field, like the anonk code already does ?
and detect at runtime if there is a native impl or if the backport
must be used.
> 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.
>   
I think a static final field is a better option.
> -- John
>   
Rémi