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

asotona · 2023-12-15T12:54:27Z

java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes.

This patch converts it to use Classfile API.

It is continuation of #10991

Any comments and suggestions are welcome.

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)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to e9bf7d57

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17121

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

Using diff file

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

Webrev

Link to Webrev Comment

…e Classfile API to generate proxy classes

bridgekeeper · 2023-12-15T12:55:12Z

👋 Welcome back asotona! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-15T12:55:50Z

@asotona The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-12-15T14:30:55Z

Webrevs

ExE-Boss · 2023-12-16T19:49:12Z

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

+                    PrimitiveTypeInfo prim = PrimitiveTypeInfo.get(parameterTypes[i]);
+                    cob.getstatic(prim.wrapperClass, "TYPE", CD_Class);


These can use a condy via ConstantBootstraps::primitiveClass(MethodHandles.Lookup, String, Class<?>), although that’s probably gonna need JDK‑8283739.

I recommend optimizing in another patch, as this patch aims to preserve code parity.

mlchung · 2023-12-19T20:45:58Z

Can you include a summary of the performance comparison before and after the patch per the comment in #10991.

asotona · 2023-12-20T20:04:47Z

Can you include a summary of the performance comparison before and after the patch per the comment in #10991.

I've found significant performance regressions when running these recommended benchmark.
Performance improvements are now in progress.

Thanks for the reminder.

mlchung · 2023-12-20T20:38:00Z

Can you explain the changes to resolve the performance regressions?

asotona · 2023-12-21T01:33:29Z

Profiling of the benchmarks revealed several slowdowns:

many expensive conversions from Class<?> to ClassDesc to ClassEntry, or even more expensive MethodTypeDesc
building proxy class from scratch from symbols also involves a lot of String concatenations, hashing, encoding and comparisons
computation of stack maps is also still expensive
SplitConstantPool was ineffective in some specific cases

Proposed performance improvements use pre-generated template class and each proxy is then transformed with share constant pool.
Frequently used constant pool entries are materialized in the template class constant pool to avoid expensive conversions, comparisons and hashing.
Stack maps are generated manually.

Performance fixes related to ClassFile API SplitConstantPool and StackCounter may be separated into another PR.
For now I keep them together to measure synergic effect.

Full benchmark numbers will be ready soon.

asotona · 2023-12-21T01:59:45Z

Here are actual numbers (_22 is before this patch):

Benchmark                      Mode  Cnt      Score      Error  Units
ProxyPerf.genIntf_1            avgt    5  18354.110 ?  696.706  ns/op
ProxyPerf.genIntf_1_22         avgt    5  15744.305 ? 1032.092  ns/op
ProxyPerf.genStringsIntf_3     avgt    5  26388.493 ? 2614.877  ns/op
ProxyPerf.genStringsIntf_3_22  avgt    5  21431.752 ?  398.627  ns/op
ProxyPerf.genZeroParams        avgt    5  17627.978 ?  456.046  ns/op
ProxyPerf.genZeroParams_22     avgt    5  15411.519 ? 6426.321  ns/op
ProxyPerf.getPrimsIntf_2       avgt    5  23862.592 ? 2274.386  ns/op
ProxyPerf.getPrimsIntf_2_22    avgt    5  19148.768 ?  648.304  ns/op
Finished running test 'micro:.+ProxyPerf.+'

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java

liach · 2023-12-23T12:40:33Z

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java

-                dcb.methodInfo.methodName().stringValue(),
-                dcb.methodInfo.methodTypeSymbol(),
+                dcb.methodInfo.methodName(),
+                dcb.methodInfo.methodType(),


Can you enlighten me on why this actually leads to a performance improvement? Don't we already generate the methods with MethodTypeDesc symbols in ProxyGenerator so that they should be cached?

This code is part of the ClassFile API’s internals, and so it doesn’t have access to ProxyGenerator’s cached MethodTypeDescs, only the underlying Utf8Entry, so it’d need to be parsed.

I see that now Class return type and Class array parameter types are directly converted to Strings instead of MTDs. I think that we should really run ProxyGenerator with JFR to profile and see the results (for instance, old ASM has a stackmap generation penalty due to parsing method descriptors on the fly compared to CF API, despite CF's slower overall performance due to allocations like Optional)

These changes are based on profiling of the benchmarks.
StackCounter was not used in any performance critical applications and profiling revealed some obvious bottlenecks.

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java

ExE-Boss · 2023-12-23T15:24:55Z

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java

-                dcb.methodInfo.methodName().stringValue(),
-                dcb.methodInfo.methodTypeSymbol(),
+                dcb.methodInfo.methodName(),
+                dcb.methodInfo.methodType(),


This code is part of the ClassFile API’s internals, and so it doesn’t have access to ProxyGenerator’s cached MethodTypeDescs, only the underlying Utf8Entry, so it’d need to be parsed.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

…ounter.java Co-authored-by: ExE Boss <[email protected]>

mlchung

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

mlchung · 2024-01-05T21:03:28Z

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

+            generateConstructor(clb);
+            generateLookupAccessor(clb);
+            var cp = clb.constantPool();
+            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?

I've rewritten the initialization to use indexes inside an array.
Thanks.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

mlchung · 2024-01-05T23:55:53Z

Profiling of the benchmarks revealed several slowdowns:

* many expensive conversions from `Class<?>` to `ClassDesc` to `ClassEntry`, or even more expensive `MethodTypeDesc`

* building proxy class from scratch from symbols also involves a lot of `String` concatenations, hashing, encoding and comparisons

* computation of stack maps is also still expensive

* `SplitConstantPool` was ineffective in some specific cases

Do you think these also cause the performance overhead in converting java.lang.invoke to use ClassFile API (#17108)?

liach

Also StackMapDecoder::initFrameFromLocals(ClassEntry, ...) can benefit from 2 changes:

Iterate MTD by index instead of creating copies
Pass ClassEntry from reader/writer constant pool to ObjectVerificationTypeInfoImpl instead of using the temporary pool

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

liach · 2024-01-07T18:18:32Z

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java

-                        for (var arg : mDesc.parameterList()) {
-                            addStackSlot(-TypeKind.from(arg).slotSize());
-                        }
+                        addStackSlot(-countMethodStack(nameAndType.type(), true));


Can use something like:

var mtd = Util.methodTypeSymbol(nameAndType); addStackSlot(Util.slotSize(mtd.returnType()) - Util.parameterSlots(mtd));

if you stick with MTD.

Calculation of MTD for each method invocation is extremely ineffective way to just count the size.

asotona · 2024-01-08T13:39:45Z

I've separated ClassFile API performance related improvements into
#17306

This reverts commit c6b761a.

…l/StackCounter.java" This reverts commit c8f1d30.

This reverts commit 41e8793.

This reverts commit 7d0da2c.

This reverts commit b7a60ae.

This reverts commit 0dc63d4.

Co-authored-by: Mandy Chung <[email protected]>

asotona · 2024-01-08T15:31:21Z

Profiling of the benchmarks revealed several slowdowns:
* many expensive conversions from `Class<?>` to `ClassDesc` to `ClassEntry`, or even more expensive `MethodTypeDesc`

* building proxy class from scratch from symbols also involves a lot of `String` concatenations, hashing, encoding and comparisons

* computation of stack maps is also still expensive

* `SplitConstantPool` was ineffective in some specific cases
Do you think these also cause the performance overhead in converting java.lang.invoke to use ClassFile API (#17108)?

Yes, I think it affects all performance critical applications.

asotona · 2024-01-08T15:46:12Z

Also StackMapDecoder::initFrameFromLocals(ClassEntry, ...) can benefit from 2 changes:

Iterate MTD by index instead of creating copies

I'll add this fix to the performance improvements PR, thanks!

Pass ClassEntry from reader/writer constant pool to ObjectVerificationTypeInfoImpl instead of using the temporary pool

Unfortunately initi frame is implicit and the entries may not exist in the constant pool at all.

mlchung

The change looks fine. Please rerun the benchmark after JDK-8323183 is integrated. I made JDK-8294961 blocked by JDK-8323183 to ensure it goes in after it.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

openjdk · 2024-01-09T19:18:08Z

@asotona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: mchung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Co-authored-by: Mandy Chung <[email protected]>

bridgekeeper · 2024-02-07T16:29:24Z

@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!

asotona · 2024-03-06T06:14:06Z

/integrate

openjdk · 2024-03-06T06:15:11Z

Going to push as commit bee50cd.
Since your change was applied there have been 28 commits pushed to the master branch:

96bfe61: 8326458: Menu mnemonics don't toggle in Windows LAF when F10 is pressed
fcf48ab: 8327391: Add SipHash attribution file
b665fe3: 6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns
a7280d1: 8311002: missing @SInCE info in 21 files in jdk.security.auth
721bfee: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries
809995b: 8174269: Remove COMPAT locale data provider from JDK
c6641c7: 8326831: Clarify test harness control variables in make help
3d106cb: 8325139: JFR SwapSpace event - add free swap space information on Linux when running in a container environment
c00c939: 8327364: Parallel: Remove unused ParallelCompactData::add_obj
98f0b86: 8319690: [AArch64] C2 compilation hits offset_ok_for_immed: assert "c2 compiler bug"
... and 18 more: https://git.openjdk.org/jdk/compare/0583f7357480c0500daa82f490b2fcc05f2fb65a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-06T06:15:17Z

@asotona Pushed as commit bee50cd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use th…

0267d65

…e Classfile API to generate proxy classes

asotona mentioned this pull request Dec 15, 2023

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

Closed

3 tasks

openjdk bot added the core-libs [email protected] label Dec 15, 2023

asotona marked this pull request as ready for review December 15, 2023 14:25

openjdk bot added the rfr Pull request is ready for review label Dec 15, 2023

ExE-Boss reviewed Dec 16, 2023

View reviewed changes

asotona added 3 commits December 20, 2023 17:47

performance improvements

3080d24

StackCounter performance boost

0dc63d4

performance improvements

a21fb26

asotona added 2 commits December 21, 2023 01:21

SplitConstantPool performance fix

b7a60ae

performance improvements

75e31e3

stsypanov reviewed Dec 21, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java Outdated Show resolved Hide resolved

asotona added 2 commits December 21, 2023 13:45

applied the recommended changes

7d0da2c

minor StackCounter fix

41e8793

liach reviewed Dec 23, 2023

View reviewed changes

ExE-Boss reviewed Dec 23, 2023

View reviewed changes

liach reviewed Dec 24, 2023

View reviewed changes

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java Outdated Show resolved Hide resolved

asotona and others added 2 commits January 3, 2024 12:50

Update src/java.base/share/classes/jdk/internal/classfile/impl/StackC…

c8f1d30

…ounter.java Co-authored-by: ExE Boss <[email protected]>

StackCounter fix

c6b761a

mlchung reviewed Jan 5, 2024

View reviewed changes

liach reviewed Jan 7, 2024

View reviewed changes

asotona mentioned this pull request Jan 8, 2024

8323183: ClassFile API performance improvements #17306

Closed

3 tasks

asotona and others added 9 commits January 8, 2024 14:48

Revert "StackCounter fix"

4c353bd

This reverts commit c6b761a.

Revert "Update src/java.base/share/classes/jdk/internal/classfile/imp…

3e0083f

…l/StackCounter.java" This reverts commit c8f1d30.

Revert "minor StackCounter fix"

2f7de56

This reverts commit 41e8793.

Revert "applied the recommended changes"

1000bd7

This reverts commit 7d0da2c.

Revert "SplitConstantPool performance fix"

575ead3

This reverts commit b7a60ae.

Revert "StackCounter performance boost"

2e50f84

This reverts commit 0dc63d4.

applied suggested changes

a582914

Apply suggestions from code review

8034091

Co-authored-by: Mandy Chung <[email protected]>

initialization of template entries by index

e9bf7d5

mlchung approved these changes Jan 9, 2024

View reviewed changes

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jan 9, 2024

Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java

48c2e6b

Co-authored-by: Mandy Chung <[email protected]>

Merge branch 'master' into JDK-8294961-proxy

40f99d1

openjdk bot added the integrated Pull request has been integrated label Mar 6, 2024

openjdk bot closed this Mar 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 6, 2024

		PrimitiveTypeInfo prim = PrimitiveTypeInfo.get(parameterTypes[i]);
		cob.getstatic(prim.wrapperClass, "TYPE", CD_Class);

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

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

Uh oh!

Conversation

asotona commented Dec 15, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Dec 15, 2023

Uh oh!

openjdk bot commented Dec 15, 2023

Uh oh!

mlbridge bot commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

ExE-Boss Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung commented Dec 19, 2023 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asotona commented Dec 20, 2023

Uh oh!

mlchung commented Dec 20, 2023 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asotona commented Dec 21, 2023

Uh oh!

asotona commented Dec 21, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mlchung left a comment • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlchung Jan 5, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlchung commented Jan 5, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asotona commented Jan 8, 2024

Uh oh!

asotona commented Dec 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 15, 2023 •

edited

Loading

ExE-Boss Dec 16, 2023 •

edited

Loading

mlchung commented Dec 19, 2023 •

edited by bridgekeeper bot

Loading

mlchung commented Dec 20, 2023 •

edited by bridgekeeper bot

Loading

mlchung left a comment •

edited by bridgekeeper bot

Loading

mlchung Jan 5, 2024 •

edited by bridgekeeper bot

Loading

mlchung commented Jan 5, 2024 •

edited by bridgekeeper bot

Loading

mlchung left a comment •

edited by bridgekeeper bot

Loading

openjdk bot commented Jan 9, 2024 •

edited

Loading