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

[Vladimir Ivanov]

Thanks for review, Paul.

Best regards,
Vladimir Ivanov

On 19.11.2019 21:25, Paul Sandoz wrote:
> +1
> 
>> On Nov 19, 2019, at 10:12 AM, Vladimir Ivanov <vladimir.x.ivanov at 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.
>