RFR: JDK-8242451 : ensure semantics of non-capturing lambdas are preserved independent of execution mode

[Mandy Chung]

Hi Gilles,

Additional comments:

215 try {
216 return new 
ConstantCallSite(Lookup.IMPL_LOOKUP.findStaticGetter(innerClass, 
LAMBDA_INSTANCE_FIELD, invokedType.returnType()));
217 } catch (ReflectiveOperationException e) {
218 throw new LambdaConversionException("Exception finding constructor", e);
219 }


This should use caller instead Lookup.IMPL_LOOKUP (as I suggested in my 
previous repl).   The exception message should be "Exception finding " + 
LAMBDA_INSTANCE_FIELD + " static field".

418 private void generateStaticField() {


I would rename this to generateClassInitializer since this adds 
"<clinit>" and initializes the static field.

Since this patch caches a singleton instance in the generated class, it 
could apply to the eager initialization case as well and replace the 
current use of core reflection to new an instance except that the target 
of the returning callsite would always be the singleton object (the 
result of invoking the static getter method handle).  I wonder if there 
is any performance difference.  This is just a thought that we can file 
a JBS issue to follow up.

Can you add a test case for this fix?

Mandy

On 6/23/20 11:08 AM, Gilles Duboscq wrote:
> In 8232806, a system property was introduce to disable eager initialization of the classes generated by the InnerClassLambdaMetafactory (`jdk.internal.lambda.disableEagerInitialization`).
>
> However, when `disableEagerInitialization` is true, even for non-capturing lambdas, the capturing lambda path that uses a method handle to the constructor is taken.
> This helps prevent eager initialization but also has the side effect of always returning a fresh instance of the lambda for every invocation instead of a singleton.
> While this is allowed by the specs, this might change the behaviour of some (incorrect) programs that assume a singleton is used for non-capturing lambdas.
>
> I propose to keep the effects of the `disableEagerInitialization` related to initialization only.
>
> Webrev: https://cr.openjdk.java.net/~gdub/8242451/webrev.0/
> Issue: https://bugs.openjdk.java.net/browse/JDK-8242451
>
> The concrete issue we are seeing with changing both aspects at the same time is that when disableEagerInitialization for static analysis in GraalVM's native-image, some programs start to miss-behave because of assumptions about the object identity of lambdas.
>
>
> Note that `disableEagerInitialization` is still ineffective in the following situations:
> * when `useImplMethodHandle` is true, i.e., when a MethodHanlde is used to call the target because the generated hidden class is missing the necessary access rights.
>    (the implementation require setting a static field on the generated class which causes it to be initialized, Class Data could help in the future in that case)
> * for non-capturing lambdas when the caller (and generated) class is in the `java.lang.invoke` or `sun.invoke.util` package.
>    (because `findStaticGetter` will eagerly initialize the holder class if it is from those packages, see DirectMethodHandle#shouldBeInitialized)
>
> Those are acceptable rare corner cases.
>
> Thanks,
>   Gilles