On Thu, 6 Jun 2024 10:16:14 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 two additional > commits since the last revision: > > - reverted static initialization of ConstantPoolBuilder and CP entries > - fixed naming conventions src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 616: > 614: final ClassDesc classDesc = ClassDesc.of(className0); > 615: final ClassDesc superClassDesc = > classDesc(speciesData.deriveSuperClass()); > 616: return ClassFile.of().build(classDesc, clb -> { We need a confirmation here that these BMH species are only initialized after LMF is ready. @cl4es Is a dependency on LMF from BMH safe? src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 771: > 769: final int TMODS = TRANSFORM_MODS.get(whichtm); > 770: clb.withMethod(TNAME, methodDesc(TTYPE), (TMODS & > ACC_PPP) | ACC_FINAL, mb -> { > 771: mb.withFlags((TMODS & ACC_PPP) | ACC_FINAL) Isn't this redundant as we've specified it in initial withMethod call already? src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 784: > 782: // mix them up and load them for the > transform helper: > 783: List<Var> helperArgs = > speciesData.deriveTransformHelperArguments(transformMethods.get(whichtm), > whichtm, targs, tfields); > 784: List<Class<?>> helperTypes = new > ArrayList<>(helperArgs.size()); Can we convert helperTypes here to List<ClassDesc> so we construct invokeBasicType as MethodTypeDesc below? src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 564: > 562: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER) > 563: > .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC) > 564: > .with(SourceFileAttribute.of(className.substring(className.lastIndexOf('/') + > 1))); Maybe we can keep the classDesc from ofInternalName and use its displayName. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 87: > 85: > 86: private static final String[] EMPTY_STRING_ARRAY = new String[0]; > 87: private static final ClassDesc[] EMPTY_CLASSDESC_ARRAY = new > ClassDesc[0]; Suggestion: private static final ClassDesc[] EMPTY_CLASSDESC_ARRAY = ConstantUtils.EMPTY_CLASSDESC; Need an import. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 96: > 94: } > 95: }; > 96: record MethodBody(Consumer<CodeBuilder> code) implements > Consumer<MethodBuilder> { Why do we have these 2 instead of a noop record field builder consumer (flags is already set in withField, and MethodBody should just be direct usage of withMethodBody) Seems the problem is in CF implemetnation side. Then these should be part of CF implementation details. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 214: > 212: for (int i = 0; i < parameterCount; i++) { > 213: argNames[i] = "arg$" + (i + 1); > 214: argDescs[i] = classDesc(factoryType.parameterType(i)); We should declare global ad-hoc classDesc and methodDesc methods in ConstantUtils, that way if we speed up our conversions all users benefit and the generators don't need to define these conversions everywhere. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 470: > 468: MethodVisitor mv = cw.visitMethod(ACC_PRIVATE + ACC_FINAL, > 469: NAME_METHOD_WRITE_OBJECT, > DESCR_METHOD_WRITE_OBJECT, > 470: null, SER_HOSTILE_EXCEPTIONS); The exceptions attribute is now lost on the migrated methods. Is that ok? It's private and only accessed by reflection so I think there's no real impact. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 566: > 564: > 565: static ClassDesc implClassDesc(Class<?> cls) { > 566: return cls.isHidden() ? > ReferenceClassDescImpl.ofValidatedBinaryName(cls.getName()) I recommend just returning null or a dummy value if the cls is hidden; in this case, implMethodClassDesc can safely be null, as implementation must go through condy. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 74: > 72: private static final ClassDesc CD_LambdaForm_Name = > ReferenceClassDescImpl.ofValidated("Ljava/lang/invoke/LambdaForm$Name;"); > 73: private static final ClassDesc CD_Object_array = > ReferenceClassDescImpl.ofValidated("[Ljava/lang/Object;"); > 74: private static final ClassDesc CD_LoopClauses_array = > ReferenceClassDescImpl.ofValidated("Ljava/lang/invoke/MethodHandleImpl$LoopClauses;"); Suggestion: private static final ClassDesc CD_LoopClauses = ReferenceClassDescImpl.ofValidated("Ljava/lang/invoke/MethodHandleImpl$LoopClauses;"); The use site anticipates this to be a class or interface instead of an array type. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 94: > 92: }; > 93: > 94: record MethodBody(Consumer<CodeBuilder> code) implements > Consumer<MethodBuilder> { Same withMethodBody remark src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 281: > 279: clb.withFlags(ACC_FINAL | ACC_SUPER) > 280: .withSuperclass(INVOKER_SUPER_DESC) > 281: .withVersion(CLASSFILE_VERSION, 0) We can remove version (and maybe superclass) configuration as it's already the same as the defaults from CF API. Suggestion: src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 1148: > 1146: } > 1147: > 1148: private void emitPopInsn(CodeBuilder cob, BasicType type) { We need a TypeKind-aware pop in CodeBuilder too :( ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629360525 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629410742 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629414434 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629420425 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629437086 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629430093 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629439711 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629519618 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629533698 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629536765 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629537897 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1629542392 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1637040340
