RFR: JDK-8242451 : ensure semantics of non-capturing lambdas are preserved independent of execution mode
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
On 6/23/2020 2:08 PM, 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 need to reiterate that such programs are broken, and coddling such programs is likely to backfire in the long run. When we switch to using inline classes for lambda proxies, for example, programs depending on the identity of lambda proxies likely will break, and tough luck on them. The JLS was exceedingly clear from day 1 that the identity behavior of a lambda proxy is an implementation detail that should be _expected_ to change. All that said, the change here seems fairly restrained, and I understand there are other motivations here. I have a few concerns, which I think should be easily addressed: - I would prefer not to use Lookup.IMPL_LOOKUP. We have been on a campaign to _reduce_ the use of IMPL_LOOKUP in code like this, both based on "principle of least privilege" as well as because we would like to move this code out of JLI into `java.lang.invoke` as soon as practical (this has been queued up behind some other changes). Is there a translation strategy you can see that doesn't require IMPL_LOOKUP? It seems reasonable to make the field accessible, since it is a cached instance that is freely dispensed to anyone who asks anyway. The hidden class generation produces a Lookup for the class as part of the process; getting access to it might complicate the patch a little bit, but not doing so is creating fresh technical debt in the way of a refactor we have been trying to get to for a while. - I'm having a hard time reconciling this patch against the JDK tip, which incorporates the changes for Hidden Classes. Was this patch done against an older version, or is this patch destined only for an update release? - Assuming we can resolve the above, I would like Mandy to review these as she has most recently been in this code (eliminating the dependence on `Unsafe::defineAnonymousClass` with the newly supported "hidden classes.")
On 6/23/20 11:38 AM, Brian Goetz wrote:
On 6/23/2020 2:08 PM, 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 need to reiterate that such programs are broken, and coddling such programs is likely to backfire in the long run. When we switch to using inline classes for lambda proxies, for example, programs depending on the identity of lambda proxies likely will break, and tough luck on them.
Agree. Such programs will break sooner or later. Do you have any plan to act on fixing these programs?
The JLS was exceedingly clear from day 1 that the identity behavior of a lambda proxy is an implementation detail that should be _expected_ to change.
All that said, the change here seems fairly restrained, and I understand there are other motivations here. I have a few concerns, which I think should be easily addressed:
- I would prefer not to use Lookup.IMPL_LOOKUP. We have been on a campaign to _reduce_ the use of IMPL_LOOKUP in code like this, both based on "principle of least privilege" as well as because we would like to move this code out of JLI into `java.lang.invoke` as soon as practical (this has been queued up behind some other changes).
LMF and its implementation classes do not use IMPL_LOOKUP any more. I have a patch to reduce other use of IMPL_LOOKUP even further.
Is there a translation strategy you can see that doesn't require IMPL_LOOKUP? It seems reasonable to make the field accessible, since it is a cached instance that is freely dispensed to anyone who asks anyway. The hidden class generation produces a Lookup for the class as part of the process; getting access to it might complicate the patch a little bit, but not doing so is creating fresh technical debt in the way of a refactor we have been trying to get to for a while.
You can use the caller Lookup. The hidden class is a nestmate of the caller class and the caller Lookup can access private fields of the hidden class.
- I'm having a hard time reconciling this patch against the JDK tip, which incorporates the changes for Hidden Classes. Was this patch done against an older version, or is this patch destined only for an update release?
- Assuming we can resolve the above, I would like Mandy to review these as she has most recently been in this code (eliminating the dependence on `Unsafe::defineAnonymousClass` with the newly supported "hidden classes.")
I'll post further comment. 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. Mandy
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
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
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?
Progress continues on Valhalla, and this is clearly where things are going, but integration is not imminent. But, the main point here is: if the goal is to protect broken programs that make incorrect identity assumptions, said protection is inherently short-term; when Valhalla lands, all these programs will be broken, and there will be no interest in punishing the other 99.999% of code just to continue to coddle them. So we should be clear that this is at most a short-term reprieve.
On 8/25/20 3:16 AM, Gilles Duboscq wrote:
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?
It can simply verify if the static field is only present when disableEagerInitialization is true; otherwise such static field should not exist.
I have updated the webrev: https://cr.openjdk.java.net/~gdub/8242451/webrev.1/
Looks okay to me. Nit: `Class::descriptorString` can be used instead of BytecodeDescriptor::unparse. 418 String lambdaTypeDescriptor = BytecodeDescriptor.unparse(invokedType.returnType());
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?
As Brian replied, integration of valhalla is not imminent. JDK-8205668 tracks the work to make lambda proxies as inline classes for the valhalla repo (*not* main line). Mandy
Hi Mandy, Thanks for your review. I have added a test as you suggested and switched to `.descriptorString()`. https://cr.openjdk.java.net/~gdub/8242451/webrev.2/ Gilles On 25/08/2020 19:23, Mandy Chung wrote:
On 8/25/20 3:16 AM, Gilles Duboscq wrote:
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?
It can simply verify if the static field is only present when disableEagerInitialization is true; otherwise such static field should not exist.
I have updated the webrev: https://cr.openjdk.java.net/~gdub/8242451/webrev.1/
Looks okay to me.
Nit: `Class::descriptorString` can be used instead of BytecodeDescriptor::unparse.
418 String lambdaTypeDescriptor = BytecodeDescriptor.unparse(invokedType.returnType());
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?
As Brian replied, integration of valhalla is not imminent. JDK-8205668 tracks the work to make lambda proxies as inline classes for the valhalla repo (*not* main line).
Mandy
On 8/26/20 10:16 AM, Gilles Duboscq wrote:
Hi Mandy,
Thanks for your review. I have added a test as you suggested and switched to `.descriptorString()`.
Looks fine. This test is a javac test. I'm including compiler-dev if anyone has any comment on it. Mandy
participants (3)
-
Brian Goetz
-
Gilles Duboscq
-
Mandy Chung