RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v2]
Chen Liang
liach at openjdk.org
Sat Dec 16 17:12:50 UTC 2023
On Fri, 15 Dec 2023 12:26:50 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 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
More information about the core-libs-dev
mailing list