On Thu, 4 Apr 2024 09:20:39 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 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 10 additional > commits since the last revision: > > - Reversion of ClassBuilder changes > - Merge branch 'master' into JDK-8294960-invoke > - Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - added missing comment > - fixed InnerClassLambdaMetafactory for hidden caller classes > - performance improvements > - consolidation of descriptors handling > - InvokerBytecodeGenerator::emit... improvements > - 8294960: Convert java.base/java.lang.invoke package to use the Classfile > API to generate lambdas and method handlers Overall looks good to me. How is the performance result compared to using ASM? src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 32: > 30: import java.lang.classfile.attribute.SourceFileAttribute; > 31: import java.lang.constant.ClassDesc; > 32: import static java.lang.constant.ConstantDescs.*; Nit: this static imports can be moved to line 50 grouped with other static imports. src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 53: > 51: import static java.lang.invoke.MethodHandleStatics.*; > 52: import static java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP; > 53: import static java.lang.classfile.ClassFile.*; Nit: nice to sort these in alphabetical order - this before line 50. src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 606: > 604: TRANSFORM_MODS = List.of(tms.toArray(new Integer[0])); > 605: } > 606: private static final MethodTypeDesc MTD_transformHelper = > MethodTypeDesc.of(CD_MethodHandle, CD_int); Suggestion: private static final MethodTypeDesc MTD_TRANFORM_HELPER = MethodTypeDesc.of(CD_MethodHandle, CD_int); src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 72: > 70: static final String INVOKERS_HOLDER_CLASS_NAME = > INVOKERS_HOLDER.replace('/', '.'); > 71: static final String BMH_SPECIES_PREFIX = > "java.lang.invoke.BoundMethodHandle$Species_"; > 72: private static final ClassFile CC = ClassFile.of(); `ClassFile.of()` returns the ClassFile with default context which is a singleton object. Is this static variable needed? src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 526: > 524: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER) > 525: > .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC) > 526: > .with(SourceFileAttribute.of(clb.constantPool().utf8Entry(className.substring(className.lastIndexOf('/') > + 1)))); Is it because of performance reason to use `SourceFileAttribute.of(Utf8Entry)` rather than `SourceFileAttribute.of(String)`? If so, a comment to explain would help. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 36: > 34: import java.lang.classfile.ClassBuilder; > 35: import java.lang.classfile.ClassFile; > 36: import static java.lang.classfile.ClassFile.*; Nit: group static imports in line 52 following this file convention. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 41: > 39: import java.lang.constant.ClassDesc; > 40: import java.lang.constant.ConstantDesc; > 41: import static java.lang.constant.ConstantDescs.*; Nit: group static imports following this file convention src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 57: > 55: import java.lang.classfile.constantpool.FieldRefEntry; > 56: import java.lang.classfile.constantpool.InterfaceMethodRefEntry; > 57: import java.lang.classfile.constantpool.MethodRefEntry; Nit: group these imports with the above import java.lang.classfile... src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 28: > 26: package java.lang.invoke; > 27: > 28: import static java.lang.classfile.ClassFile.*; Nit: move this static import to line 64 following this file convention. src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java line 75: > 73: UNBOX_DOUBLE = unbox(CD_Number, > "doubleValue", CD_double); > 74: > 75: static private TypeKind primitiveTypeKindFromClass(Class<?> type) { Suggestion: private static TypeKind primitiveTypeKindFromClass(Class<?> type) { ------------- PR Review: https://git.openjdk.org/jdk/pull/17108#pullrequestreview-2004669143 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568015336 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568015888 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568016797 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568013634 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1567990362 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568022324 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568034426 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568034885 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1567986889 PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1568031735
