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

[Adam Sotona]

On Tue, 16 Apr 2024 22:49:28 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> 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
>
> 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.

Fixed, thank you.

> 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.

Fixed, thank you.

> 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);

Fixed, thank you.

> 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?

Right, it is no more necessary.

> 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.

It is redundant relic. Removed, thank you.

> 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.

Fixed, thank you.

> 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

Fixed, thank you.

> 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...

Fixed, thank you.

> 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.

Fixed, thank you.

> 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) {

Fixed, thank you.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024148
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024311
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024488
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569023878
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569023198
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024574
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024826
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024982
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569022391
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024668