8324050: Issue store-store barrier after re-materializing objects during deoptimization

[vnkozlov]

Added missing store-store barrier when we re-materialize scalar replaced object during deoptimization.

I also removed redundant #if COMPILER2_OR_JVMCI guards which were leftover from JDK-8312579 changes. It added Vector API support to Graal and changed #ifdef COMPILER2 to these #if. But this code is already under these ifs.

Tested tier1-3, scope, stress.

No new regression test. I think it is "almost" impossible to hit this issue because there is a lot of VM's runtime code between the code which rematerialize scalar-replaced objects during deoptimization and a code in Interpreter which is executed after deoptimization and which may execute a store instruction that makes these objects accessible by other threads.

---

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-8324050: Issue store-store barrier after re-materializing objects during deoptimization (Bug - P3)

Reviewers

• Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 49d44048
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17503

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kvn! 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]

@vnkozlov 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.

[mlbridge]

Webrevs

• 01: Full - Incremental (392135ed)
• 00: Full (49d44048)

[dean-long]

This seems like the right place for normal deoptimization, but I'm worried that EscapeBarrier::deoptimize_objects() makes these objects visible to JVMTI without calling reassign_fields().

[vnkozlov]

I see that EscapeBarrier::deoptimize_objects_internal() calls Deoptimization::deoptimize_objects_internal() which calls rematerialize_objects() which does reallocation and fields reassignment. It will execute this barrier.

[dean-long]

Thanks, I missed that.

[openjdk]

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

8324050: Issue store-store barrier after re-materializing objects during deoptimization

Reviewed-by: dlong, shade

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

• df370d7: 8314329: AgeTable: add is_clear() & allocation spec, and relax assert to allow use of 0-index slot
• 0d8543d: 8324065: Daylight saving information for Africa/Casablanca are incorrect
• c9cacfb: 8323657: Compilation of snippet results in VerifyError at runtime with --release 9 (and above)
• bde650f: 8322282: Incorrect LoaderConstraintTable::add_entry after JDK-8298468
• be943a9: 8321984: IGV: Upgrade to Netbeans Platform 20
• d3b2ac1: 8314186: runtime/8176717/TestInheritFD.java failed with "Log file was leaked"
• 72f1990: 8323057: Recoverable errors may be reported before unrecoverable errors when annotation processing is skipped
• c84af49: 8324129: C2: Remove some ttyLocker usages in preparation for JDK-8306767
• fd37262: 8323748: RISC-V: Add Zfh probe code
• 76afa02: 8322572: AllocationMergesTests.java fails with "IRViolationException: There were one or multiple IR rule failures."
• ... and 6 more: https://git.openjdk.org/jdk/compare/f1b73350c237021c04ceac2f29f1f378630bd651...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.

[merykitty]

This seems similar to a recent discussion. There, it is decided that a release barrier would be safer. Should we do it similarly here? Thanks.

[shipilev]

It looks good, but let's not put unrelated changes together? I think the COMPILER2_OR_JVMCI should come in as a separate atomic change. This will, for example, allow to cleanly backport storestore additions without looking back whether the vector support enablement hunks make sense.

[vnkozlov]

This seems similar to a recent discussion. There, it is decided that a release barrier would be safer. Should we do it similarly here? Thanks.

I think, if decided, it should be done in separate RFE uniformly in all places: Interpreter, C1, C2 and here in deoptimization code.

[vnkozlov]

It looks good, but let's not put unrelated changes together? I think the COMPILER2_OR_JVMCI should come in as a separate atomic change. This will, for example, allow to cleanly backport storestore additions without looking back whether the vector support enablement hunks make sense.

Thank you, Aleksey

Right, backports. I removed COMPILER2_OR_JVMCI changes.

[vnkozlov]

Thank you, Dean, Aleksey and Quan for reviews and comments.

[vnkozlov]

/integrate

[openjdk]

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

• df370d7: 8314329: AgeTable: add is_clear() & allocation spec, and relax assert to allow use of 0-index slot
• 0d8543d: 8324065: Daylight saving information for Africa/Casablanca are incorrect
• c9cacfb: 8323657: Compilation of snippet results in VerifyError at runtime with --release 9 (and above)
• bde650f: 8322282: Incorrect LoaderConstraintTable::add_entry after JDK-8298468
• be943a9: 8321984: IGV: Upgrade to Netbeans Platform 20
• d3b2ac1: 8314186: runtime/8176717/TestInheritFD.java failed with "Log file was leaked"
• 72f1990: 8323057: Recoverable errors may be reported before unrecoverable errors when annotation processing is skipped
• c84af49: 8324129: C2: Remove some ttyLocker usages in preparation for JDK-8306767
• fd37262: 8323748: RISC-V: Add Zfh probe code
• 76afa02: 8322572: AllocationMergesTests.java fails with "IRViolationException: There were one or multiple IR rule failures."
• ... and 6 more: https://git.openjdk.org/jdk/compare/f1b73350c237021c04ceac2f29f1f378630bd651...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vnkozlov Pushed as commit 52523d3.

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

[theRealAph]

This seems similar to a recent discussion. There, it is decided that a release barrier would be safer. Should we do it similarly here? Thanks.

I think, if decided, it should be done in separate RFE uniformly in all places: Interpreter, C1, C2 and here in deoptimization code.

That would be the wrong thing to do, because the risk that leads us (possibly? - discuss) to need release can only happen in an optimizing compiler. AFAIK...