Hi Mandy,
Thanks for the comments.
On 24/06/2020 02:56, Mandy Chung wrote:
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".
Done.
418 private void generateStaticField() {
I would rename this to generateClassInitializer since this adds "<clinit>" and
initializes the static field.
Done
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?
I could write a test that generates different output depending on whether a
singleton or a fresh instance is returned.
Then i can compare the output when running with
`jdk.internal.lambda.disableEagerInitialization` set to `true` and `false`.
What is the recommended way of comparing 2 runs like that in jtreg?
I have updated the webrev: https://cr.openjdk.java.net/~gdub/8242451/webrev.1/
From your other mail:
Should this patch be a workaround to existing releases rather than the main
line? As Brian mentions, lambda proxy class may become inline class in
valhalla repo (Roger has a patch already). The earlier fixing those programs
the better.
Indeed if we know this is landing in this cycle in the main repo there's no
point with my fix. Could you point me at the issue number or mail thread where
this patch is being discussed?
Gilles
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
