RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
Chen Liang
liach at openjdk.org
Wed Jun 12 20:45:30 UTC 2024
On Thu, 6 Jun 2024 10:16:14 GMT, Adam Sotona <asotona at 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
More information about the core-libs-dev
mailing list