Hi Claes,
It might be ok as you say, but then I don't understand the need to
pre-resolve() NamedFunctions in original code. Do you?
Regards, Peter
On Nov 12, 2015 6:44 PM, "Claes Redestad" <claes.redes...@oracle.com> wrote:
> Hi,
>
> On 2015-11-12 17:26, Peter Levart wrote:
>
>> Hi Claes,
>>
>> I have one concern...
>>
>> 645 private static NamedFunction getConstantFunction(int idx) {
>> 646 NamedFunction function = FUNCTIONS[idx];
>> 647 if (function != null) {
>> 648 return function;
>> 649 }
>> 650 return setCachedFunction(idx, makeConstantFunction(idx));
>> 651 }
>> 652
>> 653 private static synchronized NamedFunction setCachedFunction(int
>> idx, final NamedFunction function) {
>> 654 // Simulate a CAS, to avoid racy duplication of results.
>> 655 NamedFunction prev = FUNCTIONS[idx];
>> 656 if (prev != null) {
>> 657 return prev;
>> 658 }
>> 659 FUNCTIONS[idx] = function;
>> 660 function.resolve();
>> 661 return function;
>> 662 }
>>
>>
>> Above is a classical double-checked locking idiom, but it is not using
>> volatile variable to publish the NamedFunction instance. I wonder if this
>> is safe. Even if the FUNCTIONS[idx] slot was a volatile variable, you
>> would publish new instance before resolving it. Is it OK to publish
>> unresolved NamedFunction(s)? There is a NamedFunction.resolvedHandle()
>> instance method that makes sure NamedFunction is resolved before returning
>> a MethodHandle, but there are also usages of dereferencing
>> NamedFunction.resolvedHandle field directly in code. Are you sure that such
>> unresolved or almost resolved instance of NamedFunction is never used in
>> such places where NamedFunction.resolvedHandle field is dereferenced
>> directly?
>>
>> In original code those NamedFunctions were resolved in static initializer
>> so they were published properly.
>>
>
> I think we're relying on the fact that relevant field, member, is final
> and thus will be published correctly.
>
> resolvedHandle will be set to null in the constructor we're using anyhow:
>
> NamedFunction(Method method) {
> this(new MemberName(method));
> }
> ...
> NamedFunction(MemberName member) {
> this.member = member;
> this.resolvedHandle = null;
> }
>
> I inquired myself about the rationale for using of DCL without volatile
> elsewhere in the java.lang.invoke code and was pointed to this earlier
> discussion that shed light on the assumptions and caveats:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-May/013902.html
>
> So, all in all, I think the partially unsafe publication is actually safe
> for our purposes here.
>
> Thanks!
>
> /Claes
>
