+1

> On Nov 19, 2019, at 10:12 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> 
> wrote:
> 
> 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
>  
> 

Ok, I see now in light of that context.


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

Up to you.

Paul.

Reply via email to