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

2024-04-17 Thread Adam Sotona 
On Tue, 16 Apr 2024 22:49:28 GMT, Mandy Chung  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+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
>
> 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

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

2024-04-16 Thread Mandy Chung 
On Thu, 4 Apr 2024 09:20:39 GMT, Adam Sotona  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

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

2024-04-04 Thread Adam Sotona 
> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17108/files
  - new: https://git.openjdk.org/jdk/pull/17108/files/098df109..500bb8f0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17108=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=03-04

  Stats: 598651 lines in 7745 files changed: 108259 ins; 133327 del; 357065 mod
  Patch: https://git.openjdk.org/jdk/pull/17108.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108

PR: https://git.openjdk.org/jdk/pull/17108