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.")
