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

[Peter Levart]

Hi Vladimir,

On 11/14/2015 12:59 AM, Vladimir Ivanov wrote:
> NamedFunction.resolvedHandle should be usually pre-resolved, but for 
> bootstrapping purposes it is done lazily in some cases. I don't see 
> any reason why NamedFunction.resolve() should be called on a freshly 
> created instance. Use proper constructor instead.

I agree, but I think that's just a style issue. Even if you use a proper 
constructor, the fact that resolvedHandle is not a final field means 
that such instance has to be published safely to other threads or you 
have to guarantee that resolvedHandle of such instance is accessed only 
through accessor method that checks for null and (re)initializes the 
field if it finds it still being null.

In the later case (when resolvedHandle is pre-populated and 
NamedFunction is published unsafely), it can happen that some threads 
get the pre-populated instance of resolvedHandle and other threads that 
don't yet see the pre-populated instance (because of reorderings) get 
the lazily resolved instance computed by one of them (if double-checked 
locking is used).

So if always returning a single instance from resolvedHandle() accessor 
is important, resolvedHandle field should not be pre-populated if unsafe 
publication of NamedFunction instance is exercised. What role does 
pre-resolving play? I only see it as a means to throw exceptions early. 
So perhaps an assert in selected constructors that resolves the handle 
but does not assign it to resolvedHandle field could catch any coding 
mistakes in tests for the price of double-resolving when assertions are 
enabled.

> NamedFunction(MethodHandle resolvedHandle)
>         NamedFunction(MemberName member, MethodHandle resolvedHandle)
>         NamedFunction(MethodType basicInvokerType)
>
>         // The next 3 constructors are used to break circular 
> dependencies on MH.invokeStatic, etc.
>         // Any LambdaForm containing such a member is not interpretable.
>         // This is OK, since all such LFs are prepared with special 
> primitive vmentry points.
>         // And even without the resolvedHandle, the name can still be 
> compiled and optimized.
>         NamedFunction(Method method) {
>             this(new MemberName(method));
>         }
>         NamedFunction(Field field) {
>             this(new MemberName(field));
>         }
>         NamedFunction(MemberName member) {
>             this.member = member;
>             this.resolvedHandle = null;
>         }
>
> There are 2 non-final fields in NamedFunction:
>   static class NamedFunction {
>       final MemberName member;
>       @Stable MethodHandle resolvedHandle;
>       @Stable MethodHandle invoker;
>
> Repeated initialization of NamedFunction.invoker is benign since 
> NamedFunction.computeInvoker uses synchronized 
> MethodTypeForm.setCachedMethodHandle to populate the cache.
>
> Regarding resolvedHandle, NamedFunction are usually constructed 
> pre-resolved, but for the limited number of lazily initialized 
> instances, I think we should force resolution right after 
> bootstrapping is over.

But then they are not lazily initialized any more. Or do you refer to 
eagerly initialized instances that are not pre-resolved?

> Or, if you want to save on resolvedHandle resolution, replace all 
> direct field accesses with accessor calls and synchronize when setting 
> the value.

I think there can only be two kinds of "safe" NamedFunctions:
- pre-resolved and published safely (these are the ones that are 
initialized in <clinit> of a class and published via a static field of 
the same class)
- lazily initialized, not-yet-resolved and which may be published unsafely

The refactoring that Claes is making is changind the 1st kind of 
NamedFunction(s) into the 2nd kind.

Only the 1st kind of NamedFunction(s) can be used so that their 
resolvedHandle field is directly accessed in code.

Because it is difficult to track through code which ones are which and 
it's easy to make a mistake that is hard to track down, it would be best 
to always access resolvedHandle via it's accessor anyway, to make code 
more robust to changes such as Claes'.

What do you think?

Regards, Peter

>
>
> Best regards,
> Vladimir Ivanov
>
> On 11/12/15 10:11 PM, Claes Redestad wrote:
>>
>> On 2015-11-12 19:53, Peter Levart wrote:
>>>>
>>>> Do you think this would work correctly w.r.t visibility:
>>>>
>>>>         FUNCTIONS[idx] = function;
>>>>         function.resolve();
>>>>         UNSAFE.storeFence();
>>>
>>> This does not do anything useful.
>>>
>>> What happens if you simply omit function.resolve() call. If lazy
>>> resolving works and tests pass, then perhaps it's all ok.
>>>
>>> Peter
>>
>> Actually, I simply tried backing out the DirectMethodHandle changes
>> entirely, and measured how much we'd lose by leaving that particular
>> case alone: almost nothing, as it turns out (same number of LFs are
>> generated etc). I think the safest option is to leave DirectMethodHandle
>> alone:
>>
>> http://cr.openjdk.java.net/~redestad/8142334/webrev.05/
>>
>> /Claes