Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang 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.baselineComputeavgt 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 About the security manager: the proposal to change SecurityManager to permit exporting the internal API packages would change the specification of `SecurityManger::checkPackageAccess`. Should I touch that in this patch and potentially require CSR review about SecurityManager changes, or should I go back to the original Proxy-like checks? - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1572243080
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 00:49:22 GMT, Mandy Chung wrote: >>> Apparently, calling utility methods from sun.invoke, like the >>> ensureOriginalLookup in this patch, also triggers this check. >> >> It should not trigger `checkPackageAccess` if it does not implement the >> interface AFAICT. >> >> It would trigger module-access check. `java.base` can export `sun.invoke` >> to the dynamic module (it may be worth considering putting the internal >> interface in a specific package for the dynamic module' use - `sun.invoke` >> is ok since it currently has one interface but hard to catch when someone >> adds a new class/interface in `sun.invoke` unintentionally and leak out >> sensitive API. > >> Then we still need to obtain the implemented interface and original method >> handle information every time they are queried. Having these information (or >> the method handle providing access) computed early is more convenient. > > I was thinking the wrapper instance class still implements some private > methods to get the implemented interface and original method handle which can > be accessed only `java.base` via deep reflection. > It should not trigger `checkPackageAccess` if it does not implement the > interface AFAICT. `checkPackageAccess` is also called by the application class loader: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java#L174-L189 It should make it impossible to reference for example `sun.misc.Unsafe`. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1211652282
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 00:27:24 GMT, Chen Liang wrote: > Apparently, calling utility methods from sun.invoke, like the > ensureOriginalLookup in this patch, also triggers this check. It should not trigger `checkPackageAccess` if it does not implement the interface AFAICT. It would trigger module-access check. `java.base` can export `sun.invoke` to the dynamic module (it may be worth considering putting the internal interface in a specific package for the dynamic module' use - `sun.invoke` is ok since it currently has one interface but hard to catch when someone adds a new class/interface in `sun.invoke` unintentionally and leak out sensitive API. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1210971821
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 00:48:18 GMT, Mandy Chung wrote: >>> Is it more straight-forward to keep a `ClassValue` and each >>> wrapper instance class has an entry of `TRUE` value; otherwise, `FALSE`? >> >> Then we still need to obtain the implemented interface and original method >> handle information every time they are queried. Having these information (or >> the method handle providing access) computed early is more convenient. >> >>> If we can avoid implementing sun.invoke.WrapperInstance, this package >>> access check issue would go away. Do you think you can look into it? >> >> Apparently, calling utility methods from `sun.invoke`, like the >> `ensureOriginalLookup` in this patch, also triggers this check. However, I >> still think removing the `WrapperInstance` supertype is better, as this >> makes the code cleaner in reflection. > >> Apparently, calling utility methods from sun.invoke, like the >> ensureOriginalLookup in this patch, also triggers this check. > > It should not trigger `checkPackageAccess` if it does not implement the > interface AFAICT. > > It would trigger module-access check. `java.base` can export `sun.invoke` to > the dynamic module (it may be worth considering putting the internal > interface in a specific package for the dynamic module' use - `sun.invoke` is > ok since it currently has one interface but hard to catch when someone adds a > new class/interface in `sun.invoke` unintentionally and leak out sensitive > API. > Then we still need to obtain the implemented interface and original method > handle information every time they are queried. Having these information (or > the method handle providing access) computed early is more convenient. I was thinking the wrapper instance class still implements some private methods to get the implemented interface and original method handle which can be accessed only `java.base` via deep reflection. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1210972245
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Wed, 31 May 2023 00:01:56 GMT, Mandy Chung wrote: > Is it more straight-forward to keep a `ClassValue` and each wrapper > instance class has an entry of `TRUE` value; otherwise, `FALSE`? Then we still need to obtain the implemented interface and original method handle information every time they are queried. Having these information (or the method handle providing access) computed early is more convenient. > If we can avoid implementing sun.invoke.WrapperInstance, this package access > check issue would go away. Do you think you can look into it? Apparently, calling utility methods from `sun.invoke`, like the `ensureOriginalLookup` in this patch, also triggers this check. However, I still think removing the `WrapperInstance` supertype is better, as this makes the code cleaner in reflection. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1210963364
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 22:18:17 GMT, Chen Liang wrote: >> If we can avoid implementing `sun.invoke.WrapperInstance`, this package >> access check issue would go away. Do you think you can look into it? > > I think we can probably insert a static final field in a wrapper instance > class to point to the implemented class and verify with the > `PROXY_CLASS_INFOS` ClassValue, much like VarForm field in VarHandle > implementations. This should have less reflective impact than the > annotation-based approach, and can be cached in a ClassValue as well. Should > we cache it though? Is it more straight-forward to keep a `ClassValue` and each wrapper instance class has an entry of `TRUE` value; otherwise, `FALSE`? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1210952258
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 22:05:06 GMT, Mandy Chung wrote: >>> because a conditionally-exported package is considered a >>> non-(unconditionally-)exported package. >> >> OK. I need to look into the right solution for this. The Proxy >> implementation uses null protection domain to define the proxy class whereas >> this patch uses the protection domain as the interface to define the MHProxy >> class. That's why this issue only occurs in this new implementation. > > If we can avoid implementing `sun.invoke.WrapperInstance`, this package > access check issue would go away. Do you think you can look into it? I think we can probably insert a static final field in a wrapper instance class to point to the implemented class and verify with the `PROXY_CLASS_INFOS` ClassValue, much like VarForm field in VarHandle implementations. This should have less reflective impact than the annotation-based approach, and can be cached in a ClassValue as well. Should we cache it though? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207426728
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 22:03:40 GMT, Mandy Chung wrote: >> If we keep a cache of all MHProxy classes, I think this would not need >> `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other >> class except `WrapperInstance` and the `ensureOriginalLookup` method would >> need to be in an internal class. So no problem of implementing >> `sun.invoke.WrapperInstance`. > >> because a conditionally-exported package is considered a >> non-(unconditionally-)exported package. > > OK. I need to look into the right solution for this. The Proxy > implementation uses null protection domain to define the proxy class whereas > this patch uses the protection domain as the interface to define the MHProxy > class. That's why this issue only occurs in this new implementation. If we can avoid implementing `sun.invoke.WrapperInstance`, this package access check issue would go away. Do you think you can look into it? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207416972
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:29:14 GMT, Chen Liang wrote: >>> This approach also requires the Class loading checks since the interface is >>> not unconditionally exported and fails security manager. >> >> Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy >> implements `sun.invoke.WrapperInstance`, it needs to export >> `java.base/sun.invoke` to the dynamic module.The change to >> `ClassLoader::checkPackageAccess` doesn't make sense for this. > > Yes. Unfortunately, it fails at > https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 > because a conditionally-exported package is considered a > non-(unconditionally-)exported package. If we keep a cache of all MHProxy classes, I think this would not need `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other class except `WrapperInstance` and the `ensureOriginalLookup` method would need to be in an internal class. So no problem of implementing `sun.invoke.WrapperInstance`. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207387430
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:31:40 GMT, Mandy Chung wrote: >> Yes. Unfortunately, it fails at >> https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 >> because a conditionally-exported package is considered a >> non-(unconditionally-)exported package. > > If we keep a cache of all MHProxy classes, I think this would not need > `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other > class except `WrapperInstance` and the `ensureOriginalLookup` method would > need to be in an internal class. So no problem of implementing > `sun.invoke.WrapperInstance`. > because a conditionally-exported package is considered a > non-(unconditionally-)exported package. OK. I need to look into the right solution for this. The Proxy implementation uses null protection domain to define the proxy class whereas this patch uses the protection domain as the interface to define the MHProxy class. That's why this issue only occurs in this new implementation. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207415941
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:06:36 GMT, Chen Liang wrote:
>> 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?
>
> Proxy-based impl had this check, so I'd assume MHP might want to defend
> against the same kind of improper usage.
Proxy is used by annotation implementation. I suspect that's why that check
was there.
For `MHP::asInterfaceInstance`, I would drop the `isModuleSystemInitied` check
until the API is being used during early startup. I don't expect it be called
before module system is initialized.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207391651
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:23:32 GMT, Mandy Chung wrote: >> John Rose argues against using class data for Leyden. Since class data is >> the only known way to defend against user-manufactured annotations, I >> switched back to an extra wrapper interface. This approach also requires the >> Class loading checks since the interface is not unconditionally exported and >> fails security manager. > >> This approach also requires the Class loading checks since the interface is >> not unconditionally exported and fails security manager. > > Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy > implements `sun.invoke.WrapperInstance`, it needs to export > `java.base/sun.invoke` to the dynamic module.The change to > `ClassLoader::checkPackageAccess` doesn't make sense for this. Yes. Unfortunately, it fails at https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 because a conditionally-exported package is considered a non-(unconditionally-)exported package. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207384208
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:05:39 GMT, Chen Liang wrote: > This approach also requires the Class loading checks since the interface is > not unconditionally exported and fails security manager. Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy implements `sun.invoke.WrapperInstance`, it needs to export `java.base/sun.invoke` to the dynamic module.The change to `ClassLoader::checkPackageAccess` doesn't make sense for this. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207376506
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 18:39:53 GMT, Mandy Chung wrote:
>> 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/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?
Proxy-based impl had this check, so I'd assume MHP might want to defend against
the same kind of improper usage.
> 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`?
John Rose argues against using class data for Leyden. Since class data is the
only known way to defend against user-manufactured annotations, I switched back
to an extra wrapper interface. This approach also requires the Class loading
checks since the interface is not unconditionally exported and fails security
manager.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207350363
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207349212
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang 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.baselineComputeavgt 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
> 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`.
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Mon, 8 May 2023 10:46:34 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line >> 342: >> >>> 340: >>> 341: // individual handle fields >>> 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, >>> ACC_PRIVATE | ACC_FINAL); >> >> Would a @Stable field help here? E.g if the returned functional interface >> instance is stored in a `static final` field, it should enable better >> performance? > > (actually, not sure - as the class is saved in a class value cache, so > probably adding stable won't make any difference). The class is loaded as a hidden class, so all final fields are truly final (unlike classes but like records), so @Stable is not needed. The class value cache only change performance if the cache was used in a hot path. And currently a class value cache is not considered as a cache by the JIT, i.e. the JIT does not know that if the key is a constant, the value is a constant. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187495168
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Mon, 8 May 2023 09:32:31 GMT, Maurizio Cimadamore wrote: >> 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/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line > 342: > >> 340: >> 341: // individual handle fields >> 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, >> ACC_PRIVATE | ACC_FINAL); > > Would a @Stable field help here? E.g if the returned functional interface > instance is stored in a `static final` field, it should enable better > performance? (actually, not sure - as the class is saved in a class value cache, so probably adding stable won't make any difference). - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187300990
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang 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.baselineComputeavgt 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 >> MethodHandleProxiesAsIFInstanceCreate.createCallLambda avgt 15 >> 17624.673 ± 2404.853 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createInterfaceInstance avgt 15 >> 17.554 ±0.748 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createLambda avgt 15 >> 16860.341 ± 1270.982 ns/op >> MethodHandleProxiesSuppl.testInstanceTargetavgt 15 >> 0.405 ±0.006 ns/op >> MethodHandleProxiesSuppl.testInstanceType avgt 15 >> 0.343 ±0.005 ns/op >> MethodHandleProxiesSuppl.testIsWrapperInstance avgt 15 >> 0.375 ±0.021 ns/op >> >> >> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's >> no longer applicable. >> >> [^1]: single abstract method > > 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/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 342: > 340: > 341: // individual handle fields > 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, > ACC_PRIVATE | ACC_FINAL); Would a @Stable field help here? E.g if the returned functional interface instance is stored in a `static final` field, it should enable better performance? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187237878
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
> 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.baselineComputeavgt 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 > MethodHandleProxiesAsIFInstanceCreate.createCallLambda avgt 15 > 17624.673 ± 2404.853 ns/op > MethodHandleProxiesAsIFInstanceCreate.createInterfaceInstance avgt 15 >17.554 ±0.748 ns/op > MethodHandleProxiesAsIFInstanceCreate.createLambda avgt 15 > 16860.341 ± 1270.982 ns/op > MethodHandleProxiesSuppl.testInstanceTargetavgt 15 > 0.405 ±0.006 ns/op > MethodHandleProxiesSuppl.testInstanceType avgt 15 > 0.343 ±0.005 ns/op > MethodHandleProxiesSuppl.testIsWrapperInstance avgt 15 > 0.375 ±0.021 ns/op > > > Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's > no longer applicable. > > [^1]: single abstract method 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 - Changes: - all: https://git.openjdk.org/jdk/pull/13197/files - new: https://git.openjdk.org/jdk/pull/13197/files/836d9d0b..bd21e68e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13197=15 - incr: https://webrevs.openjdk.org/?repo=jdk=13197=14-15 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13197.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13197/head:pull/13197 PR: https://git.openjdk.org/jdk/pull/13197
