On Thu, 29 Jun 2023 03:16:15 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 for revision 17: >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.503 ± 0.021 ns/op >> MethodHandleProxiesAsIFInstance.baselineCompute avgt 15 >> 0.269 ± 0.005 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.806 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 17.332 ± 0.210 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.296 ± 1.371 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 5 >> 0.419 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 5 >> 0.421 ± 0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 5 >> 1.731 ± 0.018 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 5 >> 0.418 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 5 >> 0.263 ± 0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 5 >> 0.262 ± 0.002 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 5 >> 0.267 ± 0.019 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt 5 >> 0.266 ± 0.013 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 5 >> 18.057 ± 0.182 ... > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 41 commits: > > - Code cleanup, thanks mandy! > - Merge branch 'master' of https://github.com/openjdk/jdk into > explore/mhp-iface > - 1. Change WRAPPER_TYPES to WeakHashMap to accurately determine if the > given class is > the proxy class > > Discussion: > 2. I dropped ProxyClassInfo and use Lookup just to see the simplication. > If wrapperInstanceTarget and wrapperInstanceType are frequently called, > it makes > sense to cache the method handles. > > 3. Should it use SoftReference or WeakReference? It depends if > asInterfaceInstance > will be used heavily. > > 3. I also dropped SamInfo and getStats as it can be inlined in the caller, > which > I think it's clearer to see what it does in place. > - SecurityManager fixed, minimize changes > - Merge branch 'master' into explore/mhp-iface > - Some changes per Mandy's review. SecurityManager test fails in this patch > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'master' into explore/mhp-iface > - stage, needs to fix is mh proxy instance check > - ... and 31 more: https://git.openjdk.org/jdk/compare/6f58ab2b...340b0751 This looks quite good. I have some comments below. Also pushed some of my suggestion to my [mh-proxies](https://github.com/mlchung/jdk/tree/mh-proxies) branch. I'd like to have a test to verify that `isWrapperInstance` and other `wrapperXXX` methods determine a random object whose class happens to have the same fields as the wrapper class expects but not a wrapper instance. I will review BasicTest in details next. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 226: > 224: * The hidden classes defined for I is defined in a dynamic > module M > 225: * which has access to the types referenced by the members of I > including > 226: * the parameter types, return type and exception types. This comment can be moved to where it creates the dynamic module. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 241: > 239: } > 240: > 241: private record LocalMethodInfo(MethodTypeDesc desc, List<ClassDesc> > thrown, String fieldName) {} Perhaps a simpler name `MethodInfo` Suggestion: private record MethodInfo(MethodTypeDesc desc, List<ClassDesc> thrown, String fieldName) {} src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 247: > 245: > 246: private static final Set<Class<?>> WRAPPER_TYPES = > Collections.newSetFromMap(new WeakHashMap<>()); > 247: private static final ClassValue<SoftReference<Lookup>> PROXY_LOOKUPS > = new ClassValue<>() { Let's use weak references for this implementation so that the hidden classes can be unloaded when it becomes weakly reachable. We can reevaluate this when we get more feedback on this usage. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 265: > 263: continue; > 264: > 265: String mname = m.getName(); Better documentation, more comments, would be helpful. Some suggestions below. Suggestion: // ensure it's SAM interface String mname = m.getName(); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 284: > 282: Stream.concat(DEFAULT_RETHROWS.stream(), > 283: > Arrays.stream(thrown).map(MethodHandleProxies::desc)).distinct().toList(); > 284: methods.add(new LocalMethodInfo(mtDesc, > exceptionTypeDescs, fieldName)); Suggestion: var exceptionTypeDescs = thrown.length == 0 ? DEFAULT_RETHROWS : Stream.concat(DEFAULT_RETHROWS.stream(), Arrays.stream(thrown).map(MethodHandleProxies::desc)) .distinct().toList(); methods.add(new MethodInfo(desc(mt), exceptionTypeDescs, fieldName)); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 297: > 295: > 296: var loader = intfc.getClassLoader(); > 297: Module targetModule = newDynamicModule(loader, > referencedTypes); Suggestion: // create a dynamic module for each proxy class, which needs access // to the types referenced by the members of the interface including // the parameter types, return type and exception types var loader = intfc.getClassLoader(); Module targetModule = newDynamicModule(loader, referencedTypes); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 306: > 304: ClassDesc proxyDesc = ClassDesc.of(cn); > 305: byte[] template = createTemplate(loader, proxyDesc, > desc(intfc), uniqueName, methods); > 306: var definer = new Lookup(intfc).makeHiddenClassDefiner(cn, > template, Set.of(), DUMPER); Suggestion: // define the dynamic module to the class loader of the interface var definer = new Lookup(intfc).makeHiddenClassDefiner(cn, template, Set.of(), DUMPER); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 336: > 334: private static final ClassDesc CD_IllegalAccessException = > desc(IllegalAccessException.class); > 335: private static final MethodTypeDesc MTD_void_Throwable = > MethodTypeDesc.of(CD_void, CD_Throwable); > 336: private static final MethodType > MT_void_Lookup_MethodHandle_MethodHandle = methodType(void.class, > Lookup.class, MethodHandle.class, MethodHandle.class); Several of these MethodType declarations are long lines that can be wrapped. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 347: > 345: private static final MethodTypeDesc MTD_void_String = > MethodTypeDesc.of(CD_void, CD_String); > 346: private static final String ORIGINAL_TARGET_NAME = "originalTarget"; > 347: private static final String ORIGINAL_TYPE_NAME = "originalType"; Can drop "original" prefix? Suggestion: private static final String TARGET_NAME = "target"; private static final String TYPE_NAME = "interfaceType"; src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 367: > 365: clb.withInterfaceSymbols(ifaceDesc); > 366: > 367: // individual handle fields Suggestion: // static and instance fields src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 381: > 379: }); > 380: > 381: // <init>(Lookup, MethodHandle originalHandle, MethodHandle > implHandle) Suggestion: // <init>(Lookup, MethodHandle target, MethodHandle callerBoundTarget) src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 386: > 384: cob.invokespecial(CD_Object, INIT_NAME, MTD_void); > 385: > 386: // WrapperInstance.ensureOriginalLookup Suggestion: // call ensureOriginalLookup to verify the given Lookup has access src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 391: > 389: > 390: // keep original target > 391: // this.originalTarget = originalHandle; Suggestion: // this.target = target; src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 396: > 394: cob.putfield(proxyDesc, ORIGINAL_TARGET_NAME, > CD_MethodHandle); > 395: > 396: // convert individual handles Suggestion: // adjust the callerBoundTarget to the method type for each method src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 398: > 396: // convert individual handles > 397: for (var mi : methods) { > 398: // this.handleField = implHandle.asType(xxType); Suggestion: // this.m<i> = callerBoundTarget.asType(xxType); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 410: > 408: }); > 409: > 410: clb.withMethodBody(ENSURE_ORIGINAL_LOOKUP, MTD_void_Lookup, > ACC_PRIVATE | ACC_STATIC, cob -> { Suggestion: + // void ensureOriginalLookup(Lookup) checks if the given Lookup has ORIGINAL + // access to this class, i.e. the lookup class is this class; otherwise, + // IllegalAccessException is thrown clb.withMethodBody(ENSURE_ORIGINAL_LOOKUP, MTD_void_Lookup, ACC_PRIVATE | ACC_STATIC, cob -> { test/jdk/java/lang/invoke/MethodHandleProxies/ProxyForMethodHandleTest.java line 34: > 32: import org.junit.jupiter.api.Test; > 33: > 34: /* Add `@bug` tag test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 37: > 35: public static void main(String... args) { > 36: var originalMh = MethodHandles.zero(void.class); > 37: Object o = > MethodHandleProxies.asInterfaceInstance(Untrusted.class, originalMh); Is it better to define an interface in this test? It seems that this test has nothing to do with Untrusted. It may confuse readers. test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 42: > 40: assert originalMh == untrustedTarget : "Got " + untrustedTarget; > 41: > 42: var runnableTarget = > MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Runnable.class, > originalMh)); please break this long line. Perhaps adding a local variable may make it easy to read too. test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 42: > 40: assert originalMh == untrustedTarget : "Got " + untrustedTarget; > 41: > 42: var runnableTarget = > MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Runnable.class, > originalMh)); Just to be completed, also add test cases for `isWrapperInstance` and `wrapperInstanceType`. ------------- PR Review: https://git.openjdk.org/jdk/pull/13197#pullrequestreview-1507889870 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248283212 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248281976 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248281584 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248285455 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248284374 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248285933 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248286077 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248287026 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248287738 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289389 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289527 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289943 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290172 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290731 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290809 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289730 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248292045 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248294487 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248292958 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248295035
