On Wed, 5 Apr 2023 04:50:04 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 call performance on par with >> those of lambda expressions; the creation performance is slightly less than >> that of LambdaMetafactory: >> https://jmh.morethan.io/?gist=fcb946d83ee4ac7386901795ca49b224 >> >> 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. >> >> 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 > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 13 additional commits since > the last revision: > > - Use a dumper for interface instances, other cleanup > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'master' into explore/mhp-iface > - Remove unused JavaLangReflectReflectAccess.invokeDefault > - rethrow error > - Benchmark compareing to lamda metafactory, various minor updates > - Cache the spinned class, loading is still slow > - Wrapper instance type is already available > - mark records as private > - Merge branch 'master' into explore/mhp-iface > - ... and 3 more: https://git.openjdk.org/jdk/compare/389698f9...0eec507d I think the approach taken by this patch is good. I agree with your analysis about access checking. The access checks are preformed when creating the target method handle, and if the target is a `@CallerSensitive` method, the caller will be the lookup class. So, it seems like it's not possible to e.g. turn a MethodHandle pointing at MethodHandles::lookup into an interface instance to get access to a Lookup created in the target interface's module. I also think the approach of creating a new class per method handle is good. This is currently the only way to guarantee peak performance as far as I'm aware. If only a single class is desired, this can be achieved by using a lambda expression that captures the target method handle. Though, that also requires knowing the target type statically. I think this change needs a CSR as well to document the change in behavior. If an application currently calls asInterfaceInstance many times for the same interface, the amount of classes generated will increase (potentially a lot). src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 187: > 185: throw newIllegalArgumentException("a sealed interface", > intfc.getName()); > 186: if (intfc.isHidden()) > 187: throw newIllegalArgumentException("a hidden interface", > intfc.getName()); This extra check should be reflected in the javadoc as well. (there's already a note about the interface needing to be public and not sealed). src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 200: > 198: > 199: // Interface-specific setup > 200: var info = INTERFACE_INFOS.get(intfc); // throws > IllegalArgumentException I find the use of `var` confusing here, since the type doesn't appear locally and is not well known either. Suggestion: InterfaceInfo info = INTERFACE_INFOS.get(intfc); // throws IllegalArgumentException src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 213: > 211: .defineClassAsLookup(true, List.of(mhs)); > 212: proxy = lookup.findConstructor(lookup.lookupClass(), > methodType(void.class)) > 213: .asType(methodType(Object.class)).invokeExact(); This can use `invoke` instead of an explicit `asType` and `invokeExact`. It is more or less the same. Suggestion: proxy = lookup.findConstructor(lookup.lookupClass(), methodType(void.class)).invoke(); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 225: > 223: private record LocalMethodInfo(MethodTypeDesc desc, List<ClassDesc> > thrown) {} > 224: > 225: private record InterfaceInfo(@Stable MethodType[] types, Lookup > lookup, @Stable byte[] template) {} Use of `@Stable` here is not needed. We don't constant fold through `InterfaceInfo` instances. Suggestion: private record InterfaceInfo(MethodType[] types, Lookup lookup, byte[] template) {} src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 256: > 254: } > 255: > 256: var template = createTemplate(desc(intfc), > methods.get(0).getName(), infos); I think using an explicit type would be preferable here as well Suggestion: byte[] template = createTemplate(desc(intfc), methods.get(0).getName(), infos); src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 293: > 291: > 292: // Spin an implementation class for an interface. A new class should > be defined for each handle. > 293: // constructor parameter: Array[target, mh1, mh2, ...] This comment seems outdated. The constructor doesn't have parameters atm src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 294: > 292: // Spin an implementation class for an interface. A new class should > be defined for each handle. > 293: // constructor parameter: Array[target, mh1, mh2, ...] > 294: private static byte[] createTemplate(ClassDesc ifaceDesc, String > name, List<LocalMethodInfo> methods) { 'name' is quite ambiguous here, I suggest `name` -> `methodName` src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 309: > 307: > 308: // actual implementations > 309: int i = 1; Can you give `i` a more descriptive name here? Also, can you add a comment that says why it starts at `1` (e.g. `// +1 skip original target mh`). src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 321: > 319: bcb.loadInstruction(kind, slot); > 320: slot += kind.slotSize(); > 321: } You could also use `CodeBuilder::parameterSlot` instead of computing the slot manually. test/jdk/java/lang/invoke/MethodHandleProxies/BasicTest.java line 123: > 121: assertEquals(Objects.toIdentityString(p1), p1.toString()); > 122: assertEquals(Objects.toIdentityString(p2), p2.toString()); > 123: } tbh I don't really see the point of this test since we don't override hashCode/equals/toString? test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstance.java line 82: > 80: > 81: @Benchmark > 82: public Doable lambdaCreate() throws Throwable { I know this is pre-existing, but I don't think having both call and create benchmarks in the same class is that useful. Since the time that is taken by each is very different. I suggest splitting these benchmarks into 2 files instead: one for the call benchmarks, and the other for create, and create + call benchmarks (and then change the output time units for the latter). I think the call benchmark could be fleshed out a bit more as well. It would be interesting to see these cases: 1. direct call to `doWork` (this would be the baseline) 2. call through non-constant method handle 3. call through non-constant interface instance created with lambda (existing `lambdaCall`) 4. call through non-constant interface instance created with MHP::asInterfaceInstance (existing `testCall`) 5. call through constant (`static final`) method handle 6. call through constant (`static final`) interface instance created with lambda 7. call through constant (`static final`) interface instance created with MHP::asInterfaceInstance ------------- PR Review: https://git.openjdk.org/jdk/pull/13197#pullrequestreview-1372885252 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158688369 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158515268 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158513973 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158516812 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158529119 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158551214 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158548324 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158557239 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158555913 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158585395 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1158668111
