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

[asotona]

java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes and this patch converts it to use Classfile API.

Please review.

Thank you,
Adam

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes (Sub-task - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/10991/head:pull/10991
$ git checkout pull/10991

Update a local copy of the PR:
$ git checkout pull/10991
$ git pull https://git.openjdk.org/jdk.git pull/10991/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10991

View PR using the GUI difftool:
$ git pr show -t 10991

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10991.diff

Webrev

Link to Webrev Comment

[liach]

How large is the frames list expected to be? We can probably perform binary searches if the list is too large.

[asotona]

The number of frames is usually low, however I'll try to benchmark the difference.
Thanks.

[asotona]

For ProxyGenerator the difference was insignificant, however in benchmarks transforming also more complex code it improved StackMapGenerator performance by approx. 7%.

[liach]

I have an alternative implementation in #13186 which somewhat speeds up for no-arg method types too; the code there no longer copies the parameter arrays on parameterList() calls, might help too (as a few places in Classfile API uses MethodTypeDesc.parameterList())

[bridgekeeper]

@asotona This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[liach]

Keep-alive.

I believe the changes to cache ClassDesc symbol in ClassEntry and avoid re-computing hash in Utf8Entry can be in a separate patch delivered before the current migration patch.

For the StackMapGenerator changes, I don't agree with replacing TOP_TYPE with null: currently, top type is 0, null, 0, which can be the default null instance after valhalla drops (so filling array will no longer be required at that point). Using null makes the code less readable and maintainable as well.

I also question the design to move away from MethodTypeDesc: ofDescriptor is expensive, but we can check the slots of a method type with a MethodTypeDesc easily than tokenizing and skipping the raw descriptor string, like

var slots = type.parameterCount()
for (var param : type.parameterList()) // when parameterList is optimized
    if (param.isPrimitive() && (param.descriptorString.charAt(0) == 'D' || param.descriptorString.charAt(0) == 'J')) slots++;

Same for processing invoke instructions: if we can reuse the MethodTypeDesc at invokeInstruction call sites, we don't need an ad-hoc type computation either.

[asotona]

I agree, setting this PR back to draft.
This pull request have to take into account all actual and planned improvements in Constants API.
And then it should isolate performance improvements of Classfile API into a separate pull request.
And then it should be proposed back for review with benchmarks.

[JornVernee]

WRT MethodTypeDesc descriptor strings, you might get some performance improvements out of caching the string in MethodTypeDescImpl:

    @Override
    public String descriptorString() {
        String descriptor = this.descriptor;
        if (descriptor == null) {
            this.descriptor = descriptor = MethodTypeDesc.super.descriptorString();
        }
        return descriptor;
    }

Where this.descriptor is:

@Stable private String descriptor;

Also, you can initialize this field directly in MethodTypeDescImpl::ofDescriptor.

(FWIW, I don't think doing this is worth it for ClassDesc, since it already stores the descriptor directly).

[liach]

(FWIW, I don't think doing this is worth it for ClassDesc, since it already stores the descriptor directly).

Frequently, ClassDesc is used in CONSTANT_Class_info, which takes internal names than descriptors. I have a patch at #13598 that aims to make such use cases clear and enable caching of internal names

[RogerRiggs]

Its not clear why this was changed, but if its for performance consider pre-sizing the StringBuilder.

Or consider an alternative using StringJoiner to avoid creating extra strings:

    StringJoiner sj = new StringJoiner("", "(", returnType.descriptorString());
    for (int i=0; i<parameterCount(); i++) {
        sj.add(parameterType(i).descriptorString());
    }
    sj.add(")");
    return sj.toString();  // toString performs the concatenation

[liach]

It would be better if StringJoiner can specify the number of elements it joins (we want parameterCount() + 3; unfortunately it can't.

[bridgekeeper]

@asotona This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@asotona This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[asotona]

This PR cannot be rebased or merged, so it continues as #17121