JDK-8287061: Support for rematerializing scalar replaced objects participating in allocation merges #12897
Conversation
… in allocation merges
|
👋 Welcome back cslucas! A progress list of the required criteria for merging this PR into |
|
@JohnTortugo The following labels will be automatically applied to this pull request:
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. |
|
/label add hotspot-runtime |
|
@JohnTortugo |
Webrevs
|
|
/label hotspot-compiler |
|
@merykitty |
|
@JohnTortugo this pull request can not be integrated into git checkout rematerialization-of-merges
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
merge-conflict
openjdk
bot
added
rfr
vnkozlov
left a comment
vnkozlov
left a comment
There was a problem hiding this comment.
Thank you, @JohnTortugo, for continue working on it. I will test it and do proper review late.
Of cause @iwanowww have to approve it too :)
|
You new test failed in GHA testing with 32-bit VM: |
… & create new IR class to represent scalarized merges.
|
Overall, the testing went well. (It discovered some minor issues I commented about.) |
|
Testing results (both functional and performance) are good. In addition, I tested with a guarantee that no Once you address my latest comments I'll mark the PR as reviewed. |
|
Thank you once more for the comments @iwanowww . I’ll address them asap. Can I ask what requirements are there for a product flag? |
Product flags are treated as part of public API of the JVM. So, changes in behavior have to go through CSR process. Also, a product flag has to be deprecated/obsoleted first before it can be removed which takes multiple releases to happen. Better to avoid introducing new product flags unless it is well-justified or necessary. |
|
@iwanowww - I believe I've addressed all your comments so far. Is everything still looking good? |
iwanowww
left a comment
iwanowww
left a comment
There was a problem hiding this comment.
The patch looks good.
I resubmitted testing with the latest version and the results are clean.
|
@JohnTortugo 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: 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 39 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @iwanowww) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
vnkozlov
left a comment
vnkozlov
left a comment
There was a problem hiding this comment.
The final version looks good to me too.
|
Thank you all for reviewing this PR! Your feedback made it much better! |
|
/integrate |
|
@JohnTortugo |
|
/sponsor |
|
Going to push as commit a53345a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@vnkozlov @JohnTortugo Pushed as commit a53345a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
If I understand correctly, this change was backported to older Microsoft OpenJDK versions. Is there a plan to do the same for upstream? |
6 participants
Can I please get reviews for this PR?
The most common and frequent use of NonEscaping Phis merging object allocations is for debugging information. The two graphs below show numbers for Renaissance and DaCapo benchmarks - similar results are obtained for all other applications that I tested.
With what frequency does each IR node type occurs as an allocation merge user? I.e., if the same node type uses a Phi N times the counter is incremented by N:
What are the most common users of allocation merges? I.e., if the same node type uses a Phi N times the counter is incremented by 1:
This PR adds support scalar replacing allocations participating in merges used as debug information OR as a base for field loads. I plan to create subsequent PRs to enable scalar replacement of merges used by other node types (CmpP is next on the list) subsequently.
The approach I used for rematerialization is pretty straightforward. It consists basically of the following. 1) New IR node (suggested by V. Kozlov), named SafePointScalarMergeNode, to represent a set of SafePointScalarObjectNode; 2) Each scalar replaceable input participating in a merge will get a SafePointScalarObjectNode like if it weren't part of a merge. 3) Add a new Class to support the rematerialization of SR objects that are part of a merge; 4) Patch HotSpot to be able to serialize and deserialize debug information related to allocation merges; 5) Patch C2 to generate unique types for SR objects participating in some allocation merges.
The approach I used for enabling the scalar replacement of some of the inputs of the allocation merge is also pretty straightforward: call
MemNode::split_through_phito, well, split AddP->Load* through the merge which will render the Phi useless.I tested this with JTREG tests tier 1-4 (Windows, Linux, and Mac) and didn't see regression. I also experimented with several applications and didn't see any failure. I also ran tests with "-ea -esa -Xbatch -Xcomp -XX:+UnlockExperimentalVMOptions -XX:-TieredCompilation -server -XX:+IgnoreUnrecognizedVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP" and didn't observe any related failures.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12897/head:pull/12897$ git checkout pull/12897Update a local copy of the PR:
$ git checkout pull/12897$ git pull https://git.openjdk.org/jdk.git pull/12897/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12897View PR using the GUI difftool:
$ git pr show -t 12897Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12897.diff
Webrev
Link to Webrev Comment