8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator #19598

cl4es · 2024-06-07T12:12:44Z

This PR refactors type matching in BootstrapMethodInvoker and adds a few types, seeking to improve bootstrap overheads of some ConstantBootstraps and in particular the ProxyGenerator condys generated for e.g. annotation proxies since JDK-8332457

I've adjusted the micro-benchmark added by JDK-8332457 to not only generate a proxy but also call into one of the proxied methodt (Object::hashCode).

Running org.openjdk.bench.java.lang.reflect.ProxyGenBench as a one-off startup benchmark sees significant improvement (-9% instructions, -6% cycles):

Name             Cnt           Base          Error            Test          Error         Unit  Change
Perfstartup-JMH   20        154,000 ±        8,165         148,000 ±       23,164        ms/op   1,04x (p = 0,352 )
  :.cycles            925335973,200 ± 47147600,262   842221278,800 ± 46836254,964       cycles   0,91x (p = 0,000*)
  :.instructions     2101588857,600 ± 81105850,361  1966307798,400 ± 22011043,908 instructions   0,94x (p = 0,000*)
  :.taskclock               291,500 ±       16,494         262,000 ±       15,328           ms   0,90x (p = 0,000*)
  * = significant

Number of classes loaded drops from 1096 to 1092

Running the micro regularly shows no significant difference:

Name                              Cnt   Base   Error    Test   Error  Unit  Change
ProxyGenBench.generateAndProxy100  10 26,827 ± 8,954  26,855 ± 7,531 ms/op   1,00x (p = 0,991 )
  * = significant

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-8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator (Bug - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Jorn Vernee (@JornVernee - Reviewer) 🔄 Re-review required (review applies to 532966a4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19598

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-07T12:13:22Z

👋 Welcome back redestad! 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 · 2024-06-07T12:13:47Z

@cl4es 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:

8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator

Reviewed-by: liach, jvernee

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 690 new commits pushed to the master branch:

0c1e911: 8338019: Fix simple -Wzero-as-null-pointer-constant warnings in riscv code
55c5097: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option
9f08a01: 8338010: WB_IsFrameDeoptimized miss ResourceMark
e7a0b5b: 8334780: Crash: assert(h_array_list.not_null()) failed: invariant
bfb75b9: 8336926: jdk/internal/util/ReferencedKeyTest.java can fail with ConcurrentModificationException
53c9f03: 8336849: Remove .llvm_addrsig section from JDK/VM static libraries (.a files)
9695f09: 8337683: Fix -Wconversion problem with arrayOop.hpp
78773b1: 8338015: Fix "Java Java" typo in package info file of java.lang.classfile
6a9a867: 8337994: [REDO] Native memory leak when not recording any events
4b740d8: 8225209: jdk/jfr/event/compiler/TestCodeSweeper.java fails
... and 680 more: https://git.openjdk.org/jdk/compare/a2030fff9833aba40e8c7c177151a30a0812a250...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

openjdk · 2024-06-07T12:14:08Z

@cl4es 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 · 2024-06-07T12:17:23Z

Webrevs

JornVernee · 2024-06-07T13:43:34Z

src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java

-                            .invokeExact(caller, name, (Class<?>)type, (MethodType)argv[0],
-                                    (MethodHandle)argv[1], (MethodType)argv[2]);
-                } else if (isStringConcatFactoryBSM(bsmType) && argv.length >= 1) {
+                } else if (argv.length >= 1 && bsmType == SCF_MT) {


It seems that these argv.length comparisons were here for cases where the wrong number of arguments are passed to a bootstrap method?

I don't see why check argv.length > 1 is still required here... Some of the other cases are also happy to just access argv, (which we also do here on the next line).

If you don't think this extract check is needed, please remove this source of confusion.

JornVernee · 2024-06-07T13:46:22Z

As an aside: I suppose we pre-generate all the linkers that get spun up for these invokeExactcalls already. But, we might still get a little bit of mileage out of switching to invokeBasic, which bypasses the linkers altogether. With the caveat that we have to be very careful that the call site type is correct.

liach

I recommend considering the MethodHandle, Object[] signature used by ConstantBootstrap.invoke: There had been previous proposals to use any method as a bootstrap method, and that API in particular can bridge that gap.

liach · 2024-06-07T13:45:42Z

src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java

-    private static boolean isLambdaMetafactoryIndyBSM(MethodType bsmType) {
-        return bsmType == LMF_INDY_MT;
-    }
+    private static final MethodType CONDY_GET_STATIC_FINAL_MT = MethodType.methodType(Object.class,


This type should just be called CONDY_NO_ARG as it's the most common form of condy that doesn't take any arg (nullConstant primitiveClass etc.)

Note that ConstantBootstraps::primitiveClass(Lookup, String, Class) has a static return type of Class⁠<⁠?⁠>, so the following is also needed:

/** * Exact type used of the {@link ConstantBootstraps#primitiveClass(Lookup, String, Class)} * bootstrap method. */ private static final MethodType CONDY_CLASS_NO_ARG_MT = CONDY_GET_STATIC_FINAL_MT.changeReturnType(Class.class);

It's not the intent of this PR to exhaustively enumerate all methods in ConstantBootstraps, primarily those shown to be bootstrap sensitive in some app. I've so far never seen a use of primitiveClass (and I admit being ignorant as to why this even exists as a BSM) so I don't have any reason to believe special-casing it will carry its own weight.

For its existence, I think it was the same as null, that they weren't representable by cp entries for bootstrap method args. They are now backed by PrimitiveClassDescImpl, a type of condy, and ClassFile API currently writes loadConstant(primitiveCd) as a condy.

I am sure there's a perfectly good explanation why we've done it like that rather than translating things like javac would (AFAICT a load from Integer/Short/Byte/...TYPE).

liach · 2024-06-07T13:49:58Z

src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java

-    private static boolean isLambdaMetafactoryAltMetafactoryBSM(MethodType bsmType) {
-        return bsmType == LMF_ALT_MT;
-    }
+    private static final MethodType CONDY_GET_STATIC_FINAL_CLASS_MT = MethodType.methodType(Object.class,


I recommend calling this CONDY_EXTRA_CLASS, as this is the type of arrayVarHandle.

…gnized type signatures

cl4es · 2024-06-07T18:55:02Z

As an aside: I suppose we pre-generate all the linkers that get spun up for these invokeExactcalls already. But, we might still get a little bit of mileage out of switching to invokeBasic, which bypasses the linkers altogether. With the caveat that we have to be very careful that the call site type is correct.

I'm not sure I see what linkers you're alluding to here in profiles (and I don't think we have code pre-generating all of them), and I don't see how to use invokeBasic here instead. Care to elaborate?

liach

I think Jorn is recommending to use invokeBasic with erased types as how LambdaForm does it. We can try that approach, but we need new tests to verify that bootstrap method type mismatches throw the same exceptions, so that should be another RFE.

cl4es · 2024-06-10T07:56:03Z

Sure. If you can show how you'd do that I'd be happy to test it. Maybe there are excess native linkers I'm not seeing from using invokeExact, but AFAICT there are no linkers being spun when the type match is made exact via static casts (which is what these exact matches in BMI achieves). So I remain a bit confused about what we would be able to improve.

Thanks for reviewing!

/integrate

openjdk · 2024-06-10T07:56:53Z

@cl4es This pull request has not yet been marked as ready for integration.

JornVernee · 2024-06-10T09:33:15Z

@cl4es I'm talking about the linker being spun in Invokers::invokeHandleForm. When either invoke or invokeExact is called, the VM calls MethodHandleNatives::linkMethod and we eventually end up there. That code creates a new LambdaForm that has the type check (for invokeExact) and customization check, and then forwards the arguments to invokeBasic.

The idea would be to call invokeBasic directly instead of invokeExact (similar to what we do in VarHandleGuards)

JornVernee · 2024-06-10T09:38:03Z

But any way, if you don't see those linkers being spun in profiles, maybe it's not important, and the extra safety is more worthwhile.

…ant.java

cl4es · 2024-06-10T10:06:07Z

Thanks, I'll see if I can get that working and if it has any effect as a follow-up.

Testing caught a bug introduced by a copy-paste error in 532966a, re-running a few more tests now.

bridgekeeper · 2024-08-09T00:13:06Z

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

cl4es · 2024-08-15T20:07:21Z

Since JDK-8332457 was more or less backed out there's no obvious gain from parts of this patch. I'd like to rework it and add startup tests that verify the ConstantBootstraps. Closing for now.

cl4es added 5 commits June 7, 2024 10:27

Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator

7b8d29b

Change ProxyGenBench to provoke proxy condy

3f86477

Ensure proxying for hashCode etc is 'implemented'

f9e0f12

Ensure proxying for hashCode etc is 'implemented'

5138fc5

Rename microbenchmark

21f945b

openjdk bot added the rfr Pull request is ready for review label Jun 7, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 7, 2024

JornVernee reviewed Jun 7, 2024

View reviewed changes

liach reviewed Jun 7, 2024

View reviewed changes

Adress review comments, add ConstantBootstraps#invoke to list of reco…

532966a

…gnized type signatures

liach approved these changes Jun 8, 2024

View reviewed changes

JornVernee approved these changes Jun 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 10, 2024

Copy-paste error; CONDY_INVOKE returns Object. Fixes TestDynamicConst…

2d64ae1

…ant.java

cl4es mentioned this pull request Jun 10, 2024

8333854: IllegalAccessError with proxies after JDK-8332457 #19615

Closed

3 tasks

openjdk bot removed the ready Pull request is ready to be integrated label Jul 11, 2024

liach approved these changes Aug 9, 2024

View reviewed changes

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

cl4es closed this Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator #19598

8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator #19598

cl4es commented Jun 7, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024 •

edited

Loading

openjdk bot commented Jun 7, 2024

mlbridge bot commented Jun 7, 2024 •

edited

Loading

JornVernee Jun 7, 2024 •

edited

Loading

JornVernee commented Jun 7, 2024

liach left a comment

liach Jun 7, 2024

ExE-Boss Jun 7, 2024

cl4es Jun 7, 2024

liach Jun 7, 2024

cl4es Jun 8, 2024

liach Jun 7, 2024

cl4es commented Jun 7, 2024

liach left a comment

cl4es commented Jun 10, 2024

openjdk bot commented Jun 10, 2024

JornVernee commented Jun 10, 2024 •

edited

Loading

JornVernee commented Jun 10, 2024

cl4es commented Jun 10, 2024

bridgekeeper bot commented Aug 9, 2024

cl4es commented Aug 15, 2024 •

edited

Loading

8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator #19598

8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator #19598

Conversation

cl4es commented Jun 7, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024 • edited Loading

openjdk bot commented Jun 7, 2024

mlbridge bot commented Jun 7, 2024 • edited Loading

Webrevs

JornVernee Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

JornVernee commented Jun 7, 2024

liach left a comment

Choose a reason for hiding this comment

liach Jun 7, 2024

Choose a reason for hiding this comment

ExE-Boss Jun 7, 2024

Choose a reason for hiding this comment

cl4es Jun 7, 2024

Choose a reason for hiding this comment

liach Jun 7, 2024

Choose a reason for hiding this comment

cl4es Jun 8, 2024

Choose a reason for hiding this comment

liach Jun 7, 2024

Choose a reason for hiding this comment

cl4es commented Jun 7, 2024

liach left a comment

Choose a reason for hiding this comment

cl4es commented Jun 10, 2024

openjdk bot commented Jun 10, 2024

JornVernee commented Jun 10, 2024 • edited Loading

JornVernee commented Jun 10, 2024

cl4es commented Jun 10, 2024

bridgekeeper bot commented Aug 9, 2024

cl4es commented Aug 15, 2024 • edited Loading

cl4es commented Jun 7, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 7, 2024 •

edited

Loading

mlbridge bot commented Jun 7, 2024 •

edited

Loading

JornVernee Jun 7, 2024 •

edited

Loading

JornVernee commented Jun 10, 2024 •

edited

Loading

cl4es commented Aug 15, 2024 •

edited

Loading