8301996: Move field resolution information out of the cpCache #14129

matias9927 · 2023-05-24T16:55:47Z

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

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 ResolvedFieldEntry. Verified with tier 1-9 tests.

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

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

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 2146b72f
Frederic Parain (@fparain - Reviewer) ⚠️ Review applies to 733860f3

Contributors

Gui Cao <gcao@openjdk.org>
Dingli Zhang <dzhang@openjdk.org>
Martin Doerr <mdoerr@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14129

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-24T16:57:05Z

👋 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-05-24T16:59:19Z

@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 · 2023-06-09T19:11:32Z

@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 field_entry_8301996
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 · 2023-06-15T17:31:14Z

⚠️ @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).

mlbridge · 2023-06-23T21:02:27Z

Webrevs

src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp

coleenp

This looks really good! I have a couple of very minor comments but the rewriter thing I noted should be fixed.

src/hotspot/cpu/x86/interp_masm_x86.cpp

src/hotspot/share/interpreter/interpreterRuntime.hpp

src/hotspot/share/interpreter/rewriter.cpp

src/hotspot/share/oops/cpCache.cpp

src/hotspot/share/oops/resolvedFieldEntry.cpp

src/hotspot/share/oops/resolvedFieldEntry.hpp

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp

coleenp

Looks great!

DingliZhang

From the cursory review, with some comments.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp

TheRealMDoerr · 2023-07-05T10:18:27Z

I'll take a closer look once the aarch64 code is in a good shape and see if I can provide a PPC64 implementation. I hope to find time for it in 2 weeks.

openjdk · 2023-07-05T16:08:22Z

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

8301996: Move field resolution information out of the cpCache

Co-authored-by: Gui Cao <gcao@openjdk.org>
Co-authored-by: Dingli Zhang <dzhang@openjdk.org>
Co-authored-by: Martin Doerr <mdoerr@openjdk.org>
Reviewed-by: coleenp, fparain

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

5362ec9: 8312492: Remove THP sanity checks at VM startup
e47a84f: 8312489: Increase jdk.jar.maxSignatureFileSize default which is too low for JARs such as WhiteSource/Mend unified agent jar
78f6799: 8293972: runtime/NMT/NMTInitializationTest.java#default_long-off failed with "Suspiciously long bucket chains in lookup table."
97b6883: 8295059: test/langtools/tools/javap 12 test classes use com.sun.tools.classfile library
3671d83: 8313252: Java_sun_awt_windows_ThemeReader_paintBackground release resources in early returns
b60e0ad: 8313207: Remove MetaspaceShared::_has_error_classes
408987e: 8313307: java/util/Formatter/Padding.java fails on some Locales
6fca289: 8313023: Return value corrupted when using CCS + isTrivial (mainline)
f8c2b7f: 8313231: Redundant if statement in ZoneInfoFile
807ca2d: 8313316: Disable runtime/ErrorHandling/MachCodeFramesInErrorFile.java on ppc64le
... and 171 more: https://git.openjdk.org/jdk/compare/af7f95e24ad5981c5de4b5dbf37da6f4f5e42129...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.

DingliZhang · 2023-07-10T07:10:23Z

It looks like I misunderstood what tbz does! I believe you are correct in suggesting that the andr is not necessary.

Hi, @matias9927,
Thanks for update! We have already done the adaptation for RISC-V locally and we are currently testing. I will update the test results and give the corresponding patch later.

fparain

Thank you for addressing the issues that were spotted.
Looks good to me now.

DingliZhang · 2023-07-12T01:31:04Z

It looks like I misunderstood what tbz does! I believe you are correct in suggesting that the andr is not necessary.

Hi, @matias9927, Thanks for update! We have already done the adaptation for RISC-V locally and we are currently testing. I will update the test results and give the corresponding patch later.

Hi,
@zifeihan and I have finished the RISC-V part and tier1-3 looks good.
Please help us to add the RISC-V part, thanks a lot!
DingliZhang@a14433d on this branch: https://github.com/DingliZhang/jdk/tree/pr-14129-rebase

matias9927 · 2023-07-12T15:15:15Z

It looks like I misunderstood what tbz does! I believe you are correct in suggesting that the andr is not necessary.

Hi, @matias9927, Thanks for update! We have already done the adaptation for RISC-V locally and we are currently testing. I will update the test results and give the corresponding patch later.

Hi, @zifeihan and I have finished the RISC-V part and tier1-3 looks good. Please help us to add the RISC-V part, thanks a lot! DingliZhang@a14433d on this branch: https://github.com/DingliZhang/jdk/tree/pr-14129-rebase

Thank you for your contribution! I really appreciate the help.

/contributor add @zifeihan
/contributor add @DingliZhang

openjdk · 2023-07-12T15:16:36Z

@matias9927
Contributor Gui Cao <gcao@openjdk.org> successfully added.

openjdk · 2023-07-12T15:17:25Z

@matias9927
Contributor Dingli Zhang <dzhang@openjdk.org> successfully added.

src/hotspot/cpu/riscv/templateTable_riscv.cpp

src/hotspot/cpu/x86/templateTable_x86.cpp

DingliZhang · 2023-07-22T06:03:10Z

Thank you for your update!
Similar to aarch64, it would be better to add tos_state in putfield_or_static as well on RISC-V.
Please help us to add the patch, thanks a lot!
DingliZhang@1fd29d6 on branch https://github.com/DingliZhang/jdk/tree/pr-14129-putfield_or_static

TheRealMDoerr · 2023-07-22T13:04:23Z

src/hotspot/cpu/riscv/templateTable_riscv.cpp

+        __ la(temp_reg, Address(temp_reg, in_bytes(ResolvedFieldEntry::put_code_offset())));
+      }
+      // Load-acquire the bytecode to match store-release in ResolvedFieldEntry::fill_in()
+      __ membar(MacroAssembler::AnyAny);


Why do you need this membar?

I am referencing the mapping from ARM memory operations onto RISC-V memory instructions in riscv-spec [1]:

Since RISC-V does not currently have plain load and store opcodes with aq or rl annotations, ARM load-acquire and store-release operations should be mapped using fences instead. Furthermore, in order to enforce store-release-to-load-acquire ordering, there must be a FENCE RW,RW between the store-release and load-acquire;

So I am trying to be conservative here and added this membar to enforce store-release-to-load-acquire ordering. I can remove that if we are sure this is not necessary here.

[1] https://github.com/riscv/riscv-isa-manual/blob/riscv-isa-release-1239329-2023-05-23/src/mm-eplan.adoc#armmappings

TheRealMDoerr · 2023-07-22T13:05:10Z

src/hotspot/cpu/riscv/templateTable_riscv.cpp

+    __ la(temp, Address(Rcache, in_bytes(ResolvedFieldEntry::put_code_offset())));
+  }
+  // Load-acquire the bytecode to match store-release in ResolvedFieldEntry::fill_in()
+  __ membar(MacroAssembler::AnyAny);


src/hotspot/share/oops/resolvedFieldEntry.hpp

TheRealMDoerr · 2023-07-23T10:11:35Z

Thanks for improving readability! An initial PPC64 part is here: TheRealMDoerr@612ee9d.

matias9927 · 2023-07-31T18:37:49Z

Thank you for the PPC port!
/contributor add @TheRealMDoerr

matias9927 · 2023-07-31T18:40:34Z

Thank you for the excellent reviews, comments, and suggestions @fparain, @coleenp, @offamitkumar, and @dholmes-ora!
Once again, thank you to @DingliZhang, @RealFYang, and @TheRealMDoerr for contributing the ports for this change.

Github actions and tier tests show no issues so I believe this change is safe to integrate.
/integrate

openjdk · 2023-07-31T18:40:42Z

@matias9927
Contributor Martin Doerr <mdoerr@openjdk.org> successfully added.

openjdk · 2023-07-31T18:41:44Z

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

5362ec9: 8312492: Remove THP sanity checks at VM startup
e47a84f: 8312489: Increase jdk.jar.maxSignatureFileSize default which is too low for JARs such as WhiteSource/Mend unified agent jar
78f6799: 8293972: runtime/NMT/NMTInitializationTest.java#default_long-off failed with "Suspiciously long bucket chains in lookup table."
97b6883: 8295059: test/langtools/tools/javap 12 test classes use com.sun.tools.classfile library
3671d83: 8313252: Java_sun_awt_windows_ThemeReader_paintBackground release resources in early returns
b60e0ad: 8313207: Remove MetaspaceShared::_has_error_classes
408987e: 8313307: java/util/Formatter/Padding.java fails on some Locales
6fca289: 8313023: Return value corrupted when using CCS + isTrivial (mainline)
f8c2b7f: 8313231: Redundant if statement in ZoneInfoFile
807ca2d: 8313316: Disable runtime/ErrorHandling/MachCodeFramesInErrorFile.java on ppc64le
... and 171 more: https://git.openjdk.org/jdk/compare/af7f95e24ad5981c5de4b5dbf37da6f4f5e42129...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-31T18:41:52Z

@matias9927 Pushed as commit 86783b9.

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

openjdk bot added the hotspot hotspot-dev@openjdk.org label May 24, 2023

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed merge-conflict Pull request has merge conflict with target branch labels Jun 9, 2023

8301996: Move field resolution information out of the cpCache

ea853dc

matias9927 force-pushed the field_entry_8301996 branch from 3b424ae to ea853dc Compare June 23, 2023 20:50

matias9927 changed the title ~~Field entry 8301996~~ 8301996: Move field resolution information out of the cpCache Jun 23, 2023

matias9927 marked this pull request as ready for review June 23, 2023 20:57

openjdk bot added the rfr Pull request is ready for review label Jun 23, 2023

offamitkumar reviewed Jun 24, 2023

View reviewed changes

src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp Outdated Show resolved Hide resolved

coleenp suggested changes Jun 28, 2023

View reviewed changes

Coleen and Amit comments

2146b72

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 29, 2023

coleenp approved these changes Jun 29, 2023

View reviewed changes

DingliZhang reviewed Jun 30, 2023

View reviewed changes

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

DingliZhang reviewed Jun 30, 2023

View reviewed changes

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

DingliZhang reviewed Jun 30, 2023

View reviewed changes

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

DingliZhang reviewed Jun 30, 2023

View reviewed changes

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

Merge branch 'master' into field_entry_8301996

3ca12cb

TheRealMDoerr reviewed Jul 5, 2023

View reviewed changes

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp Outdated Show resolved Hide resolved

Fixed test and is_resolved

2ef87d4

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jul 5, 2023

Register naming improved

e59773e

fparain approved these changes Jul 11, 2023

View reviewed changes

RISCV port

69c695d

Merge branch 'master' into field_entry_8301996

a5bb575

RealFYang reviewed Jul 17, 2023

View reviewed changes

src/hotspot/cpu/riscv/templateTable_riscv.cpp Outdated Show resolved Hide resolved

TheRealMDoerr reviewed Jul 21, 2023

View reviewed changes

src/hotspot/cpu/x86/templateTable_x86.cpp Outdated Show resolved Hide resolved

matias9927 added 2 commits July 21, 2023 19:59

Martin and Fei comments

51e65f7

Missing semicolon

8ae1e1c

TheRealMDoerr reviewed Jul 22, 2023

View reviewed changes

src/hotspot/share/oops/resolvedFieldEntry.hpp Outdated Show resolved Hide resolved

TheRealMDoerr reviewed Jul 22, 2023

View reviewed changes

src/hotspot/share/oops/resolvedFieldEntry.hpp Outdated Show resolved Hide resolved

matias9927 added 3 commits July 25, 2023 15:30

Rename tos to tos_state and comment improvement

0d36d2c

RISCV Fix and PPC port

14e8297

Fix tos_state

a090b48

openjdk bot added the integrated Pull request has been integrated label Jul 31, 2023

openjdk bot closed this Jul 31, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301996: Move field resolution information out of the cpCache #14129

8301996: Move field resolution information out of the cpCache #14129

matias9927 commented May 24, 2023 •

edited by openjdk bot

bridgekeeper bot commented May 24, 2023

openjdk bot commented May 24, 2023

openjdk bot commented Jun 9, 2023

openjdk bot commented Jun 15, 2023

mlbridge bot commented Jun 23, 2023 •

edited

coleenp left a comment

coleenp left a comment

DingliZhang left a comment

TheRealMDoerr commented Jul 5, 2023

openjdk bot commented Jul 5, 2023 •

edited

DingliZhang commented Jul 10, 2023

fparain left a comment

DingliZhang commented Jul 12, 2023

matias9927 commented Jul 12, 2023

openjdk bot commented Jul 12, 2023

openjdk bot commented Jul 12, 2023

DingliZhang commented Jul 22, 2023

TheRealMDoerr Jul 22, 2023

DingliZhang Jul 24, 2023

TheRealMDoerr Jul 22, 2023

TheRealMDoerr commented Jul 23, 2023

matias9927 commented Jul 31, 2023

matias9927 commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

8301996: Move field resolution information out of the cpCache #14129

8301996: Move field resolution information out of the cpCache #14129

Conversation

matias9927 commented May 24, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented May 24, 2023

openjdk bot commented May 24, 2023

openjdk bot commented Jun 9, 2023

openjdk bot commented Jun 15, 2023

mlbridge bot commented Jun 23, 2023 • edited

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

DingliZhang left a comment

Choose a reason for hiding this comment

TheRealMDoerr commented Jul 5, 2023

openjdk bot commented Jul 5, 2023 • edited

DingliZhang commented Jul 10, 2023

fparain left a comment

Choose a reason for hiding this comment

DingliZhang commented Jul 12, 2023

matias9927 commented Jul 12, 2023

openjdk bot commented Jul 12, 2023

openjdk bot commented Jul 12, 2023

DingliZhang commented Jul 22, 2023

TheRealMDoerr Jul 22, 2023

Choose a reason for hiding this comment

DingliZhang Jul 24, 2023

Choose a reason for hiding this comment

TheRealMDoerr Jul 22, 2023

Choose a reason for hiding this comment

TheRealMDoerr commented Jul 23, 2023

matias9927 commented Jul 31, 2023

matias9927 commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

openjdk bot commented Jul 31, 2023

matias9927 commented May 24, 2023 •

edited by openjdk bot

mlbridge bot commented Jun 23, 2023 •

edited

openjdk bot commented Jul 5, 2023 •

edited