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

Mon Nov 16 12:49:53 UTC 2015

The trick here is @Stable annotation.

If the @Stable field is written to non-default value in constructor, it 
should be equivalent to final field initialization.

If the field is written to later, the initialization should be either 
idempotent or properly synchronized. I'd like to note that though 
multiple writes to @Stable fields are allowed now, the behavior is 
undefined [1], so it would be better to ensure the field is written only 
once. Also, though we haven't seen any issues with NamedFunction 
initialization before (probably, because it is performed during 
bootstrap), we saw problems in other parts of the framework when 
multiple non-default values are observed (e.g. JDK-8005873 [2]).

There are 2 ways to delay initialization:
   (1) lazily instantiate NamedFunctions;
   (2) lazily initialize NamedFucntion.

Both approaches should give startup savings.

In the first case, there should be some exceptions to let the system 
bootstrap (some instances should be lazily resolved).

Second case looks cleaner, but requires proper publication as you noted.

I'm fine with (2): make NF.resolve() synchronized and access the field 
only through accessor (make the field private to guarantee that).

I don't see any need in NamedFunction caches then - it should be cheap 
to instantiate NFs.

Best regards,
Vladimir Ivanov

[1] @Stable javadoc:
"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."

[2] https://jbs.oracle.com/browse/JDK-8005873

On 11/15/15 12:29 PM, Peter Levart wrote:
> 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
>