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

[Peter Levart]

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.

Regards, Peter

On 11/12/2015 04:55 PM, Claes Redestad wrote:
>
> On 2015-11-12 14:47, Paul Sandoz wrote:
>>> On 11 Nov 2015, at 15:32, Claes Redestad <claes.redestad at oracle.com> 
>>> wrote:
>>>
>>> Paul,
>>>
>>> On 2015-11-10 11:55, Paul Sandoz wrote:
>>>> DirectMethodHandle
>>>> —
>>>>   682     private static @Stable NamedFunction[] FUNCTIONS = new 
>>>> NamedFunction[NF_LIMIT];
>>>>
>>>> Invokers
>>>> —
>>>>   442     private static @Stable NamedFunction[] FUNCTIONS = new 
>>>> NamedFunction[NF_LIMIT];
>>>>
>>>> MethodHandleImpl
>>>> —
>>>> 1627     private static @Stable NamedFunction[] FUNCTIONS = new 
>>>> NamedFunction[NF_LIMIT];
>>>>
>>>>
>>>> To be complete you could add “final”, thus it makes it clear that 
>>>> @Stable refers specifically to the array element.
>>>>
>>>> Paul.
>>> Thanks for having a look and catching this:
>>>
>>> http://cr.openjdk.java.net/~redestad/8142334/webrev.03
>>>
>>> - added final keyword to FUNCTIONS and HANDLES
>>> - added @Stable to ARRAYS, FILL_ARRAYS, and FILL_ARRAY_TO_RIGHT
>>>
>> MethodHandleImpl.java
>> —
>>
>> 1413     private static final @Stable MethodHandle[] FILL_ARRAYS = 
>> new MethodHandle[FILL_ARRAYS_COUNT + 1];
>> 1414
>> 1415     private static MethodHandle getFillArray(int count) {
>> 1416         assert (count > 0 && count <= FILL_ARRAYS_COUNT);
>>
>> Why FILL_ARRAYS_COUNT + 1 rather than FILL_ARRAYS_COUNT?
>>
>> Based on the previous code I would have expected the bounds to be:
>>
>>    0 < count < FILL_ARRAYS_COUNT
>>
>> Paul.
>
> Yes. Updated http://cr.openjdk.java.net/~redestad/8142334/webrev.03 
> in-place.
>
> /Claes