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
