RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v5]

[Mandy Chung]

On Thu, 4 Apr 2024 09:20:39 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 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+liach at users.noreply.github.com>
>  - Apply suggestions from code review
>    
>    Co-authored-by: liach <7806504+liach at 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