On Mon, 27 Mar 2023 23:34:52 GMT, Chen Liang <li...@openjdk.org> wrote:
> As John Rose has pointed out in this issue, the current j.l.r.Proxy based > implementation of MethodHandleProxies.asInterface has a few issues: > 1. Exposes too much information via Proxy supertype (and WrapperInstance > interface) > 2. Does not allow future expansion to support SAM[^1] abstract classes > 3. Slow (in fact, very slow) > > This patch addresses all 3 problems: > 1. It implements proxies with one hidden class for each requested interface > and replaced WrapperInstance inheritance with an annotation. This can avoid > unexpected passing of `instanceof`, and avoids the nasty problem of exporting > a JDK interface to a dynamic module to ensure access. > 2. This patch obtains already generated classes from a ClassValue by the > requested interface type; the ClassValue can later be updated to compute > implementation generation for abstract classes as well. > 3. This patch's generated hidden classes has acceptable call and creation > performance compared to the baseline; though the methods to access wrapper > information see huge performance drops, they are not anticipated to be used > in a very frequent basis, while the old implementation's wrapper access > methods are more optimized (2ns/op) than interface implementation methods > (6ns/op). [Oracle JDK 20 vs > this](https://jmh.morethan.io/?gists=bf98de7b2128e7e5d14e697fd9921eb9,e5115a2a8fa0a45159e15fab0d95b5d8) > > Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's > no longer applicable. Tests in `jdk/java/lang/invoke` and > `jdk/java/lang/reflect` pass. > > Alternative implementation: > [An alternative > implementation](https://github.com/openjdk/jdk/commit/72dbf9d4e01c455854d9b865cb2a47c38f37a8e0) > was to generate a proxy class for each methodhandle than sharing across > methodhandles. That implementation was abandoned for its bad proxy creation > performance, despite it having excellent call performance. [Alternative > implementation vs > this](https://jmh.morethan.io/?gists=08abb39f224574550925beb8be1b2f59,e5115a2a8fa0a45159e15fab0d95b5d8) > > In addition, I have a question: in > [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields > can be optimized as seen in > [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219). > Does it affect the execution performance of MethodHandle in hidden classes' > Condy vs. MethodHandle in regular final field in hidden classes? > > [^1]: single abstract method Also, some history. - I've worked on something similar before: https://bugs.openjdk.org/browse/JDK-8257605 The idea then was to add a new API in `MethodHandles` but since then I've been thinking that enhancing the performance of `MethodHandleProxies` might be better. The only issue I saw with that is that the current API doesn't accept a `MethodHandles.Lookup` object which would be needed to define an interface instance if the interface is in a non-exported package. Your solution just creates the lookup directly. Typically we have to be careful with injecting classes into other packages/modules since it can lead to access escalation, but maybe in this case it is fine. I don't think there's a way that this could be used to escalate access, though it seems strange that anyone could now define a class in a non-exported package/some other module. - We've also recently discussed `LambdaMetafactory` https://github.com/openjdk/jdk/pull/12493 which will produce classes strongly tied to the class loader. If we can bring MethodHandleProxies up to the same performance level as the classes produced by LambdaMetaFactory, it could serve as an alternative and people could move away from using the LambdaMetafactory runtime API (which is not really meant to be used directly). WRT that, I think the fact that the implementation proposed by this patch defines classes that are _not_ strongly tied to the defining loader is a good thing. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1486980533
