On Fri, 15 Dec 2023 12:26:50 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > added missing comment src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 318: > 316: accidentallySerializable |= !isSerializable && > Serializable.class.isAssignableFrom(i); > 317: } > 318: interfaces = new ArrayList<>(itfs); Suggestion: interfaces = List.copyOf(itfs); src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 406: > 404: public void accept(CodeBuilder cob) { > 405: cob.aload(0) > 406: .invokespecial(CD_Object, INIT_NAME, > MethodTypeDesc.of(CD_void)); Suggestion: .invokespecial(CD_Object, INIT_NAME, MTD_void); src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 548: > 546: static ClassDesc classDesc(Class<?> cls) { > 547: return cls.isHidden() ? > ClassDesc.ofInternalName(cls.getName().replace('.', '/')) > 548: : > ClassDesc.ofDescriptor(cls.descriptorString()); That said, all the usages already anticipate a valid `ClassDesc` that can be encoded in bytecode except `implClass`. You should make 2 versions of `classDesc()`, one that eagerly throws for all other usages, and another special version that handles hidden class conversion (with the same code as existing method) for https://github.com/openjdk/jdk/pull/17108/files#diff-76236a0dd20022f749d95ad5890e2bece0c4ac212d1b2ffab90ca86875f14282R173 src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 468: > 466: } > 467: > 468: private void emitStoreInsn(BasicType type, int index) { If we are going to use `localsMap` in every removed `emitStoreInsn`, perhaps we should restore it instead? Will also make it easier to move away from localsMap to CodeBuilder's tracking system. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 529: > 527: // Expects MethodHandle on the stack and actual receiver > MethodHandle in slot #0 > 528: cob.dup() > 529: .aload(0) Note for other reviewers: localsMap[0] is 0 in constructor src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 990: > 988: cob.invokevirtual(MRE_Class_isInstance); > 989: Label L_rethrow = cob.newLabel(); > 990: cob.branchInstruction(Opcode.IFEQ, L_rethrow); Suggestion: cob.ifeq(L_rethrow); src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1002: > 1000: > 1001: cob.labelBinding(L_rethrow); > 1002: cob.throwInstruction(); Suggestion: cob.athrow(); src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1131: > 1129: emitPopInsn(cob, basicReturnType); > 1130: } > 1131: cob.throwInstruction(); Suggestion: cob.athrow(); src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1145: > 1143: private static Opcode popInsnOpcode(BasicType type) { > 1144: switch (type) { > 1145: case I_TYPE: Should restore the enhanced switch syntax src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1171: > 1169: > cob.getfield(ClassDesc.ofInternalName("java/lang/invoke/MethodHandleImpl$CasesHolder"), > "cases", > 1170: CD_MethodHandle.arrayType()); > 1171: int casesLocal = extendLocalsMap(new Class<?>[] { > MethodHandle[].class }); We should look into replacing the local map with `CodeBuilder.allocateLocal` etc. in the future. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1519: > 1517: // double - d2i,i2b d2i,i2s d2i,i2c d2i > d2l d2f <-> > 1518: if (from != to && from != TypeKind.BooleanType && to != > TypeKind.BooleanType) try { > 1519: cob.convertInstruction(from, to); Need an intermediate int if you are converting from long/float/double to byte/short/char. src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2290: > 2288: int accessFlags; > 2289: try { > 2290: ClassModel cm = > java.lang.classfile.ClassFile.of().parse(bytes); No need for fully-qualified name. src/java.base/share/classes/java/lang/invoke/MethodType.java line 1295: > 1293: public Optional<MethodTypeDesc> describeConstable() { > 1294: try { > 1295: var params = new ClassDesc[parameterCount()]; We can probably handle like this: var retDesc = returnType().describeConstable(); if (retDesc.isEmpty()) return Optional.empty(); if (parameterCount() == 0) return Optional.of(MethodTypeDesc.of(retDesc.get())); var params = new ClassDesc[parameterCount()]; for (int i = 0; i < params.length; i++) { var paramDesc = parameterType(i).describeConstable(); if (paramDesc.isEmpty()) return Optional.empty(); params[i] = paramDesc.get(); } return Optional.of(MethodTypeDesc.of(returnType().get(), params)); To avoid creating exceptions and allocating array when the params array is empty. src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java line 162: > 160: } else { > 161: Class<?> src; > 162: if (arg == functional || functional.isPrimitive()) { `arg == functional` avoids a cast, though it's not in parity with the older code. Should probably restore the old cast that takes a source argument to avoid adding `==` checks to avoid casts. src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java line 379: > 377: @Override > 378: public ClassEntry classEntry(ClassDesc classDesc) { > 379: if (classDesc == ConstantDescs.CD_Object) { What causes the slow lookup of Object CE? If it's because that hashing needs to compute the substring first, I recommend changing the hashing algorithm if that's the case. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428838012 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428838938 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428845878 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428856469 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428851052 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428853927 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428854077 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428854309 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428855265 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428850744 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428857603 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428859242 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428860067 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428862861 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1428863415
