RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Peter Levart]

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