RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v7]

[Mandy Chung]

On Wed, 3 Jan 2024 12:36:26 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes.
>> 
>> This patch converts it to use Classfile API.
>> 
>> It is continuation of https://github.com/openjdk/jdk/pull/10991
>> 
>> 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:
> 
>   StackCounter fix

I think it's okay to split the fix to SplitConstantPool.java and StackCounter.java in a separate PR now.  This PR can go next once reviewed.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 28:

> 26: package java.lang.reflect;
> 27: 
> 28: import java.lang.classfile.*;

Nit:  sort the imports in alphabetical orders.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 112:

> 110:     private static final ClassModel TEMPLATE;
> 111: 
> 112:     private static final Utf8Entry UE_Method;

Nit: good to group this list of static variables by type for netter readability.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 135:

> 133:         var cc = ClassFile.of();
> 134:         var entries = new ArrayList<PoolEntry>(20);
> 135:         var q = new Object() {

It'd be helpful to add a comment to explain what this optimization is doing.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 148:

> 146:             generateLookupAccessor(clb);
> 147:             var cp = clb.constantPool();
> 148:             q.entries = new PoolEntry[] {

line 174-197 must match the order of CP entries being added.

Is there other way to avoid `q.next()` coupling with the order of CP entries in `q.entries`?  Maybe define constants for the indices to `q.entries` such that writing and reading the CP entries from the array using the constant index is easier to read and less error-prone?

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 299:

> 297:     /**
> 298:      * {@return the entries of the given type}
> 299:      * @param type the {@code Class} objects, no primitives nor arrays

Suggestion:

     * @param types the {@code Class} objects, not primitive types nor array types

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 303:

> 301:     private static ClassEntry[] toClassEntries(ConstantPoolBuilder cp, List<Class<?>> types) {
> 302:         var ces = new ClassEntry[types.size()];
> 303:         for (int i = 0; i< ces.length; i++)

Suggestion:

        for (int i = 0; i < ces.length; i++)

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 739:

> 737:             var desc = new StringJoiner("", "(", ")" + returnType.descriptorString());
> 738:             for (var pt : parameterTypes) {
> 739:                 desc.add(pt.descriptorString());

Maybe worth refactor these as `ProxyMethod::toMethodTypeDescriptorString` with the comment to explain why `MethodType::descriptorString` is not used.

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

PR Review: https://git.openjdk.org/jdk/pull/17121#pullrequestreview-1806808189
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443351490
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443352842
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443355763
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443365641
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443371360
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443370121
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443381525