On Sun, 7 May 2023 13:34:54 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 updates the WrapperInstance methods to take an `Empty` to avoid method
>> clashes
>> 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 faster than old implementation in general.
>>
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15
>> 1.483 ± 0.025 ns/op
>> MethodHandleProxiesAsIFInstance.baselineCompute avgt 15
>> 0.264 ± 0.006 ns/op
>> MethodHandleProxiesAsIFInstance.testCall avgt 15
>> 1.773 ± 0.040 ns/op
>> MethodHandleProxiesAsIFInstance.testCreate avgt 15
>> 16.754 ± 0.411 ns/op
>> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15
>> 19.609 ± 1.598 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15
>> 0.424 ± 0.024 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15
>> 1.936 ± 0.008 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15
>> 1.766 ± 0.014 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15
>> 0.414 ± 0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15
>> 0.271 ± 0.006 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15
>> 0.263 ± 0.004 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15
>> 0.266 ± 0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15
>> 0.262 ± 0.003 ns/op
>> MethodHandleProxiesAsIFInstanceCall.direct avgt 15
>> 0.264 ± 0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15
>> 18.000 ± 0.181 ns/op
>> MethodHandleProxiesAsIFInstanceCreat...
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove assertion, no longer true with teleport definition in MHP
src/hotspot/share/prims/jvm.cpp line 1019:
> 1017: }
> 1018: }
> 1019: assert(Reflection::is_same_class_package(lookup_k, ik),
I use the Lookup of the proxy interface to define a hidden class in a dynamic
module as the dynamic module has no class yet and it's defined to the class
loader of the proxy interface.
We should keep the same package check if the defined class is a normal class or
a hidden nestmate class. It only allows the lookup class be in a different
package for defining a hidden non-nestmate class. This is only internal
capability. `Lookup::defineHiddenClass` API will throw IAE if the given
bytes denotes a class in a different package than the lookup class.
+ if ((!is_hidden || is_nestmate) &&
!Reflection::is_same_class_package(lookup_k, ik)) {
+ // non-hidden class or nestmate class must be in the same package as the
Lookup class
+ THROW_MSG_0(vmSymbols::java_lang_IllegalArgumentException(), "Lookup class
and defined class are in different packages");
+ }
+
src/java.base/share/classes/java/lang/ClassLoader.java line 685:
> 683: final SecurityManager sm = System.getSecurityManager();
> 684: if (sm != null) {
> 685: if (ReflectUtil.isNonPublicProxyClass(cls) ||
> ReflectUtil.isMethodHandleProxiesClass(cls)) {
Why do you need this? `Proxy` allows non-public interface whereas
`MethodHandleProxies` only allows public interface.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 209:
> 207: if (intfc.isHidden())
> 208: throw newIllegalArgumentException("a hidden interface",
> intfc.getName());
> 209: if (!VM.isModuleSystemInited())
I don't expect this is needed. I assume you are thinking for LMF to use this
API?
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 245:
> 243: private record LocalMethodInfo(MethodTypeDesc desc, List<ClassDesc>
> thrown, String fieldName) {}
> 244:
> 245: private record ProxyClassInfo(MethodHandle constructor, Lookup
> originalLookup) {}
`ProxyClassInfo` for interface `I` will be stored in the class value of `I`.
Hence it keeps a strong reference to the generated proxy class for `I` until
`I` is unloaded. The hidden class implicitly becomes a strong link to the
defining class loader. Need to look into keeping it in a weak reference to
allow the hidden class to be unloaded independent of the lifetime of the class
loader.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 433:
> 431:
> 432: private static WrapperInstance asWrapperInstance(Object x) {
> 433: if (x instanceof WrapperInstance wrapperInstance)
In the previous version, `WrapperInstance` was not needed. Instead it checks
if the class is a MHProxy class. Did you run into any issue that you
resurrect `WrapperInstance`? Is it just because of accessing
`ensureOriginalLookup`?
test/jdk/java/lang/reflect/Proxy/ProxyForMethodHandle.java line 42:
> 40: * @run testng ProxyForMethodHandle
> 41: */
> 42: public class ProxyForMethodHandle {
This test seems to be useful. Can this be modified and moved to
`test/jdk/java/lang/invoke/MethodHandleProxies`?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207189226
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207185214
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207191315
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207194444
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207208803
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207190411
