[9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Remi Forax]

On 05/14/2014 03:15 PM, Vladimir Ivanov wrote:
>
> On 5/14/14 4:12 PM, Paul Sandoz wrote:
>>
>> On May 14, 2014, at 12:47 PM, Vladimir Ivanov 
>> <vladimir.x.ivanov at oracle.com> wrote:
>>
>>> Tobias, I agree with your evaluation.
>>>
>>
>> V. tricky one to track down!
>>
>>  From @Stable:
>>
>>   * It is (currently) undefined what happens if a field annotated as 
>> stable
>>   * is given a third value.  In practice, if the JVM relies on this 
>> annotation
>>   * to promote a field reference to a constant, it may be that the 
>> Java memory
>>   * model would appear to be broken, if such a constant (the second 
>> value of the field)
>>   * is used as the value of the field even after the field value has 
>> changed.
>>
>> I dunno if that was a contributing factor in this case.
> No, @Stable doesn't contribute to the problem.
>
>>
>>> My only concern is that @Stable doesn't work for 
>>> AtomicReferenceArray, so JIT doesn't see what is stored in the array.
>>
>> Yes, stability needs to be associated with the array elements.
>>
>>
>>> Maybe use a lock instead?
>>>
>>
>> Or perhaps use Unsafe.CAS directly within setCachedLambdaForm?
> Yes, Unsafe is another option here. But since cache updates should be 
> rare, Unsafe is an overkill here IMO - locking should be fine.

I don't get it, AtomicReferenceArray.CAS implementation uses Unsafe.CAS

>
>>
>> Also, as a consequence of using AtomicReferenceArray the following 
>> change may result in a memory barrier on some architectures:
>>
>>       public LambdaForm cachedLambdaForm(int which) {
>> -        return lambdaForms[which];
>>
>> +        return lambdaForms.get(which);
>>       }
>>
>> since lambdaForms.get will call Unsafe.getObjectVolatile.
> MTF.cachedLambaForm isn't on a fast path, so it shouldn't be a problem.
>
>> Separately, i think code that calls setCachedLambdaForm needs to be 
>> double checked to ensure that the return value is used. For example, 
>> in MethodHandleImpl.makeGuardWithCatchForm i see:
>>
>> basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform);
>>          return lform;
>>
>> which i think needs to be:
>>
>>          return 
>> basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform);
> Good catch! MTF.setCachedLambdaForm usages should be fixed as well.

IMO, forms can be a classical Java array,

*   final @Stable_LambdaForm[]_  lambdaForms;*


setCachedLambdaForm can use Unsafe.compareAndSwapObject

*   // Make sure cache entry is set only once*
*   if (UNSAFE.compareAndSwap(***lambdaForms*, which, null, form)) {*
*       return form;*
*   }
   **return cachedLambdaForm(which);*


and cachedLambdaForm can try to use the stable value,
if the stable value is not null, it's ok, otherwise we can use 
getVolatileObject

   public LambdaForm cachedLambdaForm(int which) {
     LambdaForm form = lambdaForms[which];
     if (form != null) {
       return form;
     }
     return UNSAFE.getObjectVolatile(lambdaForms, which);
   }


cheers,
Rémi

>
> Best regards,
> Vladimir Ivanov
>
>>
>> Paul.
>>
>>
>>
>>