8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry #12778

matias9927 · 2023-02-27T21:37:34Z

The current structure used to store the resolution information for invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its ambigious fields f1 and f2. This structure can hold information for fields, methods, and invokedynamics and each of its fields can hold different types of values depending on the entry.

This enhancement proposes a new structure to exclusively contain invokedynamic information in a manner that is easy to interpret and easy to extend. Resolved invokedynamic entries will be stored in an array in the constant pool cache and the operand of the invokedynamic bytecode will be rewritten to be the index into this array.

Any areas that previously accessed invokedynamic data from ConstantPoolCacheEntry will be replaced with accesses to this new array and structure. Verified with tier1-9 tests.

The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang and @zifeihan, and S390x by @offamitkumar.

This change supports the following platforms: x86, aarch64, PPC, RISCV, and S390x

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-8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to ff7f3503
Doug Simon (@dougxc - Committer) ⚠️ Review applies to c2d87e59
Frederic Parain (@fparain - Committer) ⚠️ Review applies to c2d87e59
Gui Cao (@zifeihan - Author) ⚠️ Review applies to 546291fc
Andrew Haley (@theRealAph - Reviewer) ⚠️ Review applies to ff7f3503
Fei Yang (@RealFYang - Reviewer) ⚠️ Review applies to 84ed272a
Amit Kumar (@offamitkumar - Author) ⚠️ Review applies to 84ed272a
Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to dad70dc5

Contributors

Richard Reingruber <rrich@openjdk.org>
Dingli Zhang <dzhang@openjdk.org>
Gui Cao <gcao@openjdk.org>
Amit Kumar <amitkumar@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12778

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

Using diff file

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

bridgekeeper · 2023-02-27T21:39:30Z

👋 Welcome back matsaave! 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-02-27T21:42:29Z

@matias9927 The following labels will be automatically applied to this pull request:

hotspot
serviceability

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

mlbridge · 2023-03-02T16:55:21Z

Webrevs

coleenp

This looks really good. I noted a few changes and questions.

src/hotspot/cpu/ppc/templateTable_ppc_64.cpp

src/hotspot/cpu/x86/templateTable_x86.cpp

src/hotspot/share/cds/classListParser.cpp

src/hotspot/share/ci/ciReplay.cpp

src/hotspot/share/oops/resolvedIndyEntry.hpp

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java

...nternal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java

src/hotspot/share/interpreter/bootstrapInfo.cpp

src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp

src/hotspot/cpu/x86/interp_masm_x86.cpp

src/hotspot/cpu/x86/templateTable_x86.cpp

src/hotspot/cpu/x86/interp_masm_x86.cpp

coleenp

I have a couple of very minor comments. This change is great. Thank you!

coleenp · 2023-03-13T21:05:11Z

src/hotspot/cpu/x86/templateTable_x86.cpp

 void TemplateTable::load_invoke_cp_cache_entry(int byte_no,
                                               Register method,
                                               Register itable_index,
                                               Register flags,
                                               bool is_invokevirtual,
                                               bool is_invokevfinal, /*unused*/
-                                               bool is_invokedynamic) {
+                                               bool is_invokedynamic /*unused*/) {


Can you remove the parameter since the s390 port is here?

Ok, never mind, I saw s390 port but it doesn't seem to be in these changes (?)

Ok, never mind, I saw s390 port but it doesn't seem to be in these changes (?)

It is not in these changes.
@offamitkumar is working on s390x. It is not yet finished though.
(I wasn't aware that putting the URL of this PR into a comment somewhere else adds a comment to this PR)

src/hotspot/share/oops/resolvedIndyEntry.hpp

openjdk · 2023-03-13T21:15:07Z

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

8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry

Co-authored-by: Richard Reingruber <rrich@openjdk.org>
Co-authored-by: Dingli Zhang <dzhang@openjdk.org>
Co-authored-by: Gui Cao <gcao@openjdk.org>
Co-authored-by: Amit Kumar <amitkumar@openjdk.org>
Reviewed-by: coleenp, dnsimon, fparain, gcao, aph, fyang, amitkumar, lucy

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

50a995f: 8304927: Update java/net/httpclient/BasicAuthTest.java to check basic auth over HTTP/2
ca745cb: 8291598: Matcher.appendReplacement should not create new StringBuilder instances
1683a63: 8305098: [Backout] JDK-8303912 Clean up JavadocTokenizer
fab2357: 8304498: JShell does not switch to raw mode when there is no /bin/test
1fc218c: 8303912: Clean up JavadocTokenizer
c1f5ca1: 8303623: Compiler should disallow non-standard UTF-8 string encodings
695683b: 8304880: [PPC64] VerifyOops code in C1 doesn't work with ZGC
83ce65e: 8305004: add @SPEC tags to langtools modules
32ef452: 8304884: Update Bytecodes data to be mostly compile time constants
927e674: 8300977: Retire java.io.ExpiringCache
... and 166 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...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.

coleenp · 2023-03-13T22:01:58Z

@dougxc Can you have a look at the jvmci changes in this PR also?

dougxc

As communicated to Matias earlier via email, the JVMCI changes in this PR look fine.

reinrich · 2023-03-14T09:20:54Z

@matias9927 can I ask you to merge master? There seem to be conflicts (at least I see a message "This branch has conflicts that must be resolved").
I'd like to give the change a spin in our CI testing. This requires that it can be applied on master.

zifeihan · 2023-03-14T16:15:01Z

Hi,
I have updated the riscv related code by referring to the latest aarch64 related changes, please help me to update it.
zifeihan@ca9f110
(on this branch: https://github.com/zifeihan/jdk/commits/follow_12778)

@RealFYang @DingliZhang Please help review the RISCV port code.

matias9927 · 2023-03-14T17:01:20Z

@matias9927 can I ask you to merge master? There seem to be conflicts (at least I see a message "This branch has conflicts that must be resolved"). I'd like to give the change a spin in our CI testing. This requires that it can be applied on master.

I saw that merge error but nothing came up when I tried to merge locally. The branch is updated nonetheless, so you should be able to test it now @reinrich !

calvinccheung

Looks good. Just a few minor comments.

src/hotspot/share/oops/cpCache.cpp

src/hotspot/share/prims/methodComparator.cpp

src/hotspot/share/interpreter/bootstrapInfo.cpp

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyEntry.java

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/BytecodeWithCPIndex.java

theRealAph

AArch64 looks good. Thank you.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp

src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp

src/hotspot/cpu/riscv/templateTable_riscv.cpp

zifeihan · 2023-03-27T10:17:37Z

More changes for RISC-V which resolve @RealFYang's review comments. @matias9927 Please help integrate this
zifeihan@413dd02
(on this branch: https://github.com/zifeihan/jdk/commits/12778_riscv_port_v4)
By the way, the riscv port passed the tier1-3 tests on unmatched board with no new errors introduced.

RealFYang

Updated RISC-V part looks good.

offamitkumar · 2023-03-28T08:14:14Z

Hi Matias,

s390x port is almost complete. All builds are successful & tier1 test for fast debug are complete. For other builds, tests are in progress. Please don't integrate, Wait for us 🙂

Thanks

offamitkumar

{tier1, tier2} X {fast debug, slow debug, release} testing done for s390x. PR seems clean.
@matias9927 please include port for s390x from this commit:
offamitkumar@a582f32

Thanks

offamitkumar · 2023-03-28T12:54:41Z

src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp

+    __ get_cache_index_at_bcp(size, 1, index_size);  // Load index.
+    // Get address of invokedynamic array
+    __ ld_ptr(cache, in_bytes(ConstantPoolCache::invokedynamic_entries_offset()), R27_constPoolCache);
+    // Scale the index to be the entry index * sizeof(ResolvedInvokeDynamicInfo)
+    __ sldi(size, size, log2i_exact(sizeof(ResolvedIndyEntry)));
+    __ add(cache, cache, size);


@reinrich Is there any specific reason, why you're not calling load_resolved_indy_entry() method here. On s390x build/changes are stable even with calling that helper method.

It should work if we move the addition of Array<ResolvedIndyEntry>::base_offset_in_bytes() into the other caller.

@reinrich Is there any specific reason, why you're not calling load_resolved_indy_entry() method here. On s390x build/changes are stable even with calling that helper method.

Looks like this was changed on x86_64 after I ported it to ppc. Thanks for making me aware of it.

matias9927 · 2023-03-28T15:29:24Z

/contributor add @offamitkumar

openjdk · 2023-03-28T15:29:51Z

@matias9927
Contributor Amit Kumar <amitkumar@openjdk.org> successfully added.

RealLucy

s390 changes look good.
Thank you, Amit, for working hard to get this done.

src/hotspot/cpu/s390/templateTable_s390.cpp

matias9927 · 2023-03-28T19:49:05Z

Thank you to all the reviewers for the detailed feedback, corrections, and improvements!
@theRealAph @dougxc @fparain @coleenp @RealFYang @RealLucy @turbanoff @TheRealMDoerr @calvinccheung

Also, thank you very much for completing the ports @reinrich, @DingliZhang, @zifeihan, and @offamitkumar!
/integrate

openjdk · 2023-03-28T19:50:51Z

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

50a995f: 8304927: Update java/net/httpclient/BasicAuthTest.java to check basic auth over HTTP/2
ca745cb: 8291598: Matcher.appendReplacement should not create new StringBuilder instances
1683a63: 8305098: [Backout] JDK-8303912 Clean up JavadocTokenizer
fab2357: 8304498: JShell does not switch to raw mode when there is no /bin/test
1fc218c: 8303912: Clean up JavadocTokenizer
c1f5ca1: 8303623: Compiler should disallow non-standard UTF-8 string encodings
695683b: 8304880: [PPC64] VerifyOops code in C1 doesn't work with ZGC
83ce65e: 8305004: add @SPEC tags to langtools modules
32ef452: 8304884: Update Bytecodes data to be mostly compile time constants
927e674: 8300977: Retire java.io.ExpiringCache
... and 166 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-28T19:51:03Z

@matias9927 Pushed as commit 3fbbfd1.

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

tstuefe · 2023-03-31T14:00:18Z

This obviously breaks arm, since its implementation is missing. I opened https://bugs.openjdk.org/browse/JDK-8305387 to track this. This is unfortunate since it holds work on arm in other areas, in my case for #10907.

This change supports the following platforms: x86, aarch64, PPC, RISCV, and S390x

I wonder about the explicit exclusion of arm. Every other CPU seems to be taken care of, even those Oracle does not maintain. Just curious, was there a special reason for excluding arm?

dougxc · 2023-03-31T14:28:08Z

It has also broken GraalVM Native Image. I'll open a JBS issue with a reproducer soon but here's hs-err from a slowdebug JDK build showing the problem:
hs_err_pid30379.log

matias9927 · 2023-03-31T15:34:12Z

This obviously breaks arm, since its implementation is missing. I opened https://bugs.openjdk.org/browse/JDK-8305387 to track this. This is unfortunate since it holds work on arm in other areas, in my case for #10907.

This change supports the following platforms: x86, aarch64, PPC, RISCV, and S390x

I wonder about the explicit exclusion of arm. Every other CPU seems to be taken care of, even those Oracle does not maintain. Just curious, was there a special reason for excluding arm?

There is no special reason ARM32 was excluded other than the fact no porter has picked it up yet. Fortunately I was able to get in contact with porters for the other platforms, but nobody took on the ARM port until now. Thank you for opening the issue!

tstuefe · 2023-04-03T05:57:18Z

This obviously breaks arm, since its implementation is missing. I opened https://bugs.openjdk.org/browse/JDK-8305387 to track this. This is unfortunate since it holds work on arm in other areas, in my case for #10907.

This change supports the following platforms: x86, aarch64, PPC, RISCV, and S390x

I wonder about the explicit exclusion of arm. Every other CPU seems to be taken care of, even those Oracle does not maintain. Just curious, was there a special reason for excluding arm?

There is no special reason ARM32 was excluded other than the fact no porter has picked it up yet. Fortunately I was able to get in contact with porters for the other platforms, but nobody took on the ARM port until now. Thank you for opening the issue!

I lack the time to do this atm; let's see if one of the porters can help. @bulasevich @snazarkin ?

tstuefe · 2023-04-03T06:10:38Z

I wonder about the explicit exclusion of arm. Every other CPU seems to be taken care of, even those Oracle does not maintain. Just curious, was there a special reason for excluding arm?

There is no special reason ARM32 was excluded other than the fact no porter has picked it up yet. Fortunately I was able to get in contact with porters for the other platforms, but nobody took on the ARM port until now. Thank you for opening the issue!

For future reference, we maintain a list of people working on ports: https://wiki.openjdk.org/display/HotSpot/Ports and we do have a mailing list for porters as well: porters-dev@openjdk.org . This makes it easier to find out who to contact.

Cheers, Thomas

sunny868 · 2023-04-21T02:33:37Z

src/hotspot/cpu/riscv/templateTable_riscv.cpp

+  Label resolved;
+
+  __ load_resolved_indy_entry(cache, index);
+  __ membar(MacroAssembler::AnyAny);


Why is the AnyAny barrier used here？

Hi @sunny868, I'm working on removing these unnecessary barriers. RISC-V port uses more conservative barriers like this for some reasons (e.g.: [1][2][3]), we can just remove them.

jdk/src/hotspot/cpu/riscv/templateTable_riscv.cpp

Lines 3438 to 3443 in 36ec05d

const int tags_offset = Array<u1>::base_offset_in_bytes();

__ add(t0, x10, x13);

__ la(t0, Address(t0, tags_offset));

__ membar(MacroAssembler::AnyAny);

__ lbu(t0, t0);

__ membar(MacroAssembler::LoadLoad | MacroAssembler::LoadStore);

jdk/src/hotspot/cpu/riscv/templateTable_riscv.cpp

Lines 3558 to 3563 in 36ec05d

// See if bytecode has already been quicked

__ add(t0, x13, Array<u1>::base_offset_in_bytes());

__ add(x11, t0, x9);

__ membar(MacroAssembler::AnyAny);

__ lbu(x11, x11);

__ membar(MacroAssembler::LoadLoad | MacroAssembler::LoadStore);

jdk/src/hotspot/cpu/riscv/templateTable_riscv.cpp

Lines 3614 to 3619 in 36ec05d

// See if bytecode has already been quicked

__ add(t0, x13, Array<u1>::base_offset_in_bytes());

__ add(x11, t0, x9);

__ membar(MacroAssembler::AnyAny);

__ lbu(x11, x11);

__ membar(MacroAssembler::LoadLoad | MacroAssembler::LoadStore);

8301995: Move invokedynamic resolution information out of the cpCache

4c52104

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Feb 27, 2023

matias9927 changed the title ~~8301995: Move invokedynamic resolution information out of the cpCache~~ 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry Feb 28, 2023

PPC and RISCV port

829517d

matias9927 force-pushed the resolvedIndyEntry_8301995 branch from 35bcaf8 to 829517d Compare February 28, 2023 20:56

matias9927 marked this pull request as ready for review March 2, 2023 16:50

openjdk bot added the rfr Pull request is ready for review label Mar 2, 2023

coleenp suggested changes Mar 7, 2023

View reviewed changes

fparain reviewed Mar 9, 2023

View reviewed changes

Interpreter optimization and comments

c2d87e5

coleenp reviewed Mar 13, 2023

View reviewed changes

src/hotspot/cpu/x86/interp_masm_x86.cpp Show resolved Hide resolved

coleenp approved these changes Mar 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2023

dougxc approved these changes Mar 13, 2023

View reviewed changes

fparain approved these changes Mar 13, 2023

View reviewed changes

matias9927 added 2 commits March 14, 2023 13:47

Merge branch 'master' into resolvedIndyEntry_8301995

adfdf66

Typo in comment

a3e7ca0

RISCV port update

db89222

calvinccheung reviewed Mar 14, 2023

View reviewed changes

theRealAph approved these changes Mar 24, 2023

View reviewed changes

RealFYang suggested changes Mar 25, 2023

View reviewed changes

RISCV patch and aarch64 improvement

84ed272

matias9927 requested review from TheRealMDoerr and RealFYang and removed request for TheRealMDoerr March 27, 2023 18:16

RealFYang approved these changes Mar 28, 2023

View reviewed changes

offamitkumar approved these changes Mar 28, 2023

View reviewed changes

s390 update

dad70dc

matias9927 requested a review from reinrich March 28, 2023 14:53

RealLucy approved these changes Mar 28, 2023

View reviewed changes

src/hotspot/cpu/s390/templateTable_s390.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/s390/templateTable_s390.cpp Outdated Show resolved Hide resolved

s390x NULL to nullptr

72ef047

openjdk bot added the integrated Pull request has been integrated label Mar 28, 2023

openjdk bot closed this Mar 28, 2023

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

tstuefe mentioned this pull request Mar 31, 2023

8291555: Implement alternative fast-locking scheme #10907

Closed

13 tasks

sunny868 reviewed Apr 21, 2023

View reviewed changes

	const int tags_offset = Array<u1>::base_offset_in_bytes();
	__ add(t0, x10, x13);
	__ la(t0, Address(t0, tags_offset));
	__ membar(MacroAssembler::AnyAny);
	__ lbu(t0, t0);
	__ membar(MacroAssembler::LoadLoad \| MacroAssembler::LoadStore);

	// See if bytecode has already been quicked
	__ add(t0, x13, Array<u1>::base_offset_in_bytes());
	__ add(x11, t0, x9);
	__ membar(MacroAssembler::AnyAny);
	__ lbu(x11, x11);
	__ membar(MacroAssembler::LoadLoad \| MacroAssembler::LoadStore);

8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry #12778

8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry #12778

Conversation

matias9927 commented Feb 27, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Feb 27, 2023

openjdk bot commented Feb 27, 2023

mlbridge bot commented Mar 2, 2023 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 13, 2023 • edited Loading

coleenp commented Mar 13, 2023

dougxc left a comment

Choose a reason for hiding this comment

reinrich commented Mar 14, 2023

zifeihan commented Mar 14, 2023

matias9927 commented Mar 14, 2023

calvinccheung left a comment

Choose a reason for hiding this comment

theRealAph left a comment

Choose a reason for hiding this comment

zifeihan commented Mar 27, 2023

RealFYang left a comment

Choose a reason for hiding this comment

offamitkumar commented Mar 28, 2023

offamitkumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matias9927 commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

RealLucy left a comment

Choose a reason for hiding this comment

matias9927 commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

openjdk bot commented Mar 28, 2023

tstuefe commented Mar 31, 2023

dougxc commented Mar 31, 2023

matias9927 commented Mar 31, 2023

tstuefe commented Apr 3, 2023

tstuefe commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matias9927 commented Feb 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 2, 2023 •

edited

Loading

openjdk bot commented Mar 13, 2023 •

edited

Loading