Skip to content

8301997: Move method resolution information out of the cpCache #15455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
matias9927 wants to merge 20 commits into from
Closed

8301997: Move method resolution information out of the cpCache #15455

matias9927 wants to merge 20 commits into from

Conversation

@matias9927
Copy link
Contributor

@matias9927 matias9927 commented Aug 28, 2023 •
edited
Loading

The current structure used to store the resolution information for methods, ConstantPoolCacheEntry, is difficult to interpret due to its ambigious fields f1 and f2. This structure previously held information for fields, methods, and invokedynamic calls which were all encoded into f1 and f2. Currently this structure only handles method entries, but it remains obtuse and inconsistent with recent changes.

This enhancement introduces a new data structure that stores the necessary resolution data in an intuitive an extensible manner. These resolved entries are stored in an array inside the constant pool cache in a very similar manner to invokedynamic entries in JDK-8301995.

Instances of ConstantPoolCache entry related to field resolution have been replaced with the new ResolvedMethodEntry, and ConstantPoolCacheEntry has been removed entirely. The class ResolvedMethodEntry holds resolution information for all types of invoke calls besides invokedynamic, and thus has fields that may be unused depending on the invoke code.

To streamline the review, please consider these major areas that have been changed:

  1. ResolvedMethodEntry class
  2. Rewriter for initialization of the structure
  3. cpCache for resolution
  4. InterpreterRuntime, linkResolver, and templateTable
  5. JVMCI
  6. SA

Verified with tier 1-9 tests.

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

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-8301997: Move method resolution information out of the cpCache (Sub-task - P4)

Reviewers

Contributors

  • Gui Cao <gcao@openjdk.org>
  • Fei Yang <fyang@openjdk.org>
  • Martin Doerr <mdoerr@openjdk.org>
  • Amit Kumar <amitkumar@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15455

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 28, 2023

👋 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
Copy link

openjdk bot commented Aug 28, 2023

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 28, 2023
@matias9927 matias9927 changed the title Method entry 8301997 8301997: Move method resolution information out of the cpCache Sep 6, 2023
@openjdk
Copy link

openjdk bot commented Sep 13, 2023

@matias9927 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout method_entry_8301997
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 13, 2023
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 22, 2023
@openjdk
Copy link

openjdk bot commented Sep 25, 2023

⚠️ @matias9927 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@matias9927 matias9927 force-pushed the method_entry_8301997 branch from 007b0e6 to 9f73d7e Compare October 10, 2023 19:12
@matias9927 matias9927 marked this pull request as ready for review October 16, 2023 18:09
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 16, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 16, 2023 •
edited
Loading

Webrevs

adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
adinn
adinn reviewed Oct 17, 2023
View reviewed changes
coleenp
coleenp suggested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm about halfway through.

src/hotspot/cpu/x86/interp_masm_x86.hpp

void load_resolved_indy_entry(Register cache, Register index);
void load_field_entry(Register cache, Register index, int bcp_offset = 1);
void load_method_entry(Register cache, Register index, int bcp_offset = 1);
Copy link
Contributor

@coleenp coleenp Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a future RFE, maybe we could put some of these common functions in src/share/interpreter/interp_masm.hpp. I think you need to remove the declarations for the 4 functions you removed from this and the aarch64 version.

adinn
adinn reviewed Oct 18, 2023
View reviewed changes
adinn
adinn reviewed Oct 18, 2023
View reviewed changes
@zifeihan
Copy link
Member

zifeihan commented Nov 10, 2023

Hi @matias9927 : This change [1] for aarch64 and x86 looks nice and I am trying to prepare a similar one for riscv. But I have a small question regarding this change. I see we are using r2 for cache on aarch64 which is a volatile (or caller-saved) register according to the ABI. So the value in cache won't be preserved accross the C-call in resolve_oop_handle. Will this work? Is there anything I missed? Thanks.
[1] 7d2a20c

Please note that I'm using assert(cache->is_nonvolatile(), "C-call in resolve_oop_handle"); on PPC64 which makes it reliable.

Hi Martin, Yes, I see that assertion for PPC. But my question is about aarch64 where cache could alias r2 which is an volatile/caller-save register according to the aarch64 ABI. I am not sure if this is OK. Maybe I missed anything?

@TheRealMDoerr
Copy link
Contributor

TheRealMDoerr commented Nov 10, 2023

Hi @matias9927 : This change [1] for aarch64 and x86 looks nice and I am trying to prepare a similar one for riscv. But I have a small question regarding this change. I see we are using r2 for cache on aarch64 which is a volatile (or caller-saved) register according to the ABI. So the value in cache won't be preserved accross the C-call in resolve_oop_handle. Will this work? Is there anything I missed? Thanks.
[1] 7d2a20c

Please note that I'm using assert(cache->is_nonvolatile(), "C-call in resolve_oop_handle"); on PPC64 which makes it reliable.

Hi Martin, Yes, I see that assertion for PPC. But my question is about aarch64 where cache could alias r2 which is an volatile/caller-save register according to the aarch64 ABI. I am not sure if this is OK. Maybe I missed anything?

Sorry, I was not clear enough. I wanted to suggest adding the assertion to other platforms and changing the register if needed.

@TheRealMDoerr
Copy link
Contributor

TheRealMDoerr commented Nov 10, 2023

AFAICS, resolve_oop_handle preserves volatile regs on aarch64 and x86_64, so, the current version should be ok.
I can see the following test passing:
make run-test TEST="java/lang/invoke" JTREG="VM_OPTIONS=-XX:+UseZGC -XX:+ZGenerational" on linux x86_64, aarch64 and ppc64le.

@zifeihan
Copy link
Member

zifeihan commented Nov 10, 2023

AFAICS, resolve_oop_handle preserves volatile regs on aarch64 and x86_64, so, the current version should be ok. I can see the following test passing: make run-test TEST="java/lang/invoke" JTREG="VM_OPTIONS=-XX:+UseZGC -XX:+ZGenerational" on linux x86_64, aarch64 and ppc64le.

Thank you for taking a look. I also checked the aarch64 and x86 code and now I see the difference.

@zifeihan
Copy link
Member

zifeihan commented Nov 10, 2023

@matias9927 Hi, I have prepared a similar improvement for riscv64. The volatile registers are also preserved on this platform.
15455-riscv-port-v3.diff.txt

TheRealMDoerr
TheRealMDoerr reviewed Nov 14, 2023
View reviewed changes
@offamitkumar
Copy link
Member

offamitkumar commented Nov 15, 2023

Hi @matias9927, Please add s390-Port from here: offamitkumar@3f70174

@matias9927
Copy link
Contributor Author

matias9927 commented Nov 15, 2023

Hi @matias9927, Please add s390-Port from here: offamitkumar@3f70174

Thank you for the port Amit!

/contributor add @offamitkumar

@openjdk
Copy link

openjdk bot commented Nov 15, 2023

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

@matias9927
Copy link
Contributor Author

matias9927 commented Nov 15, 2023

I believe this PR has reached a stage where it can be safely committed. Thank you to all the reviewers for your excellent feedback and thank you to all porters for your contributions!
/integrate

coleenp
coleenp approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need another approval. If so, here it is.

@openjdk
Copy link

openjdk bot commented Nov 15, 2023

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 15, 2023
@openjdk openjdk bot closed this Nov 15, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 15, 2023
@openjdk
Copy link

openjdk bot commented Nov 15, 2023

@matias9927 Pushed as commit ffa35d8.

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

@vnkozlov
Copy link
Contributor

vnkozlov commented Nov 17, 2023

@matias9927 this broke arm (32-bit) build:

/home/runner/work/jdk/jdk/src/hotspot/cpu/arm/interp_masm_arm.cpp:236:17: error: 'ConstantPoolCacheEntry' was not declared in this scope; did you mean 'ConstantPoolCache'?
  236 |   assert(sizeof(ConstantPoolCacheEntry) == 4*wordSize, "adjust code below");
      |                 ^~~~~~~~~~~~~~~~~~~~~~

I don't see GHA testing is setup for your repo.

@matias9927
Copy link
Contributor Author

matias9927 commented Nov 17, 2023

@matias9927 this broke arm (32-bit) build:

/home/runner/work/jdk/jdk/src/hotspot/cpu/arm/interp_masm_arm.cpp:236:17: error: 'ConstantPoolCacheEntry' was not declared in this scope; did you mean 'ConstantPoolCache'?
  236 |   assert(sizeof(ConstantPoolCacheEntry) == 4*wordSize, "adjust code below");
      |                 ^~~~~~~~~~~~~~~~~~~~~~

I don't see GHA testing is setup for your repo.

This patch includes changes to the interpreters which were provided by porters. Oracle is responsible for x86-64 and aarch64 so completed those and made an effort to inform porters of the upcoming change. The ARM32 port has not yet been completed and thus is not part of this patch.

The ARM32 ports have previously been handled by @bulasevich and @voitylov and they have been contacted.

@voitylov
Copy link

voitylov commented Nov 17, 2023

Noted, we'll follow up with the arm32 fix a little later. Thanks Matias!

@matias9927
Copy link
Contributor Author

matias9927 commented Nov 17, 2023

Noted, we'll follow up with the arm32 fix a little later. Thanks Matias!

Thanks for the confirmation @voitylov, I look forward to the port!

@tstuefe
Copy link
Member

tstuefe commented Nov 20, 2023 •
edited
Loading

@matias9927 Having the arm build broken is really bad. It's one thing if the VM is dead on arrival, but this shows up in everyone's GHA as a red flag. It teaches people to ignore GHAs. We require clean GHAs from outside developers; I think this sets a bad precedence.

@tstuefe tstuefe mentioned this pull request Nov 20, 2023
Closed
3 tasks
@andrew-m-leonard
Copy link

andrew-m-leonard commented Nov 20, 2023

Adoptium also unable to build jdk-22 ARM32, I have raised a bug: https://bugs.openjdk.org/browse/JDK-8320402

sunny868
sunny868 reviewed Nov 30, 2023
View reviewed changes
src/hotspot/cpu/riscv/templateTable_riscv.cpp
break;
}
// Load-acquire the bytecode to match store-release in InterpreterRuntime
__ membar(MacroAssembler::AnyAny);
Copy link
Contributor

@sunny868 sunny868 Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just using StoreLoad here is enough, right?

@algomaster99 algomaster99 mentioned this pull request Feb 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

7 more reviewers

@adinn adinn adinn approved these changes

@offamitkumar offamitkumar offamitkumar left review comments

@sunny868 sunny868 sunny868 left review comments

@TheRealMDoerr TheRealMDoerr TheRealMDoerr left review comments

@RealFYang RealFYang RealFYang left review comments

@fparain fparain fparain approved these changes

@coleenp coleenp coleenp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.