8306734: Shenandoah: Missing barriers on deoptimization path

[shipilev]

AFAICS, Loom integration (JDK-8284161) moved the Shenandoah block StackValue::create_stack_value that we added to avoid exposing bad oops during the deopt (JDK-8224522). I think that was done to avoid interaction with barrier healing acting on derived pointer bases (?), so it employed the NativeAccess::load_oop trick to still call into GC barriers without healing. Then, the refactoring for ZGC (JDK-8296875) redid these paths, but dropped the NativeAccess::load_oop completely, which exposed Shenandoah to a missing barrier.

Serial, Parallel, G1 are not affected by this, because they do not need load-barriers. Not sure why ZGC is not affected by this, but probably because it does the barrier injection in another place (see JDK-8224675).

There are two ways to fix it: with the Shenandoah-specific barrier injection, or reinstating the NativeAccess::load_oop trick Loom integration originally did. I preferred to keep the fix GC-agnostic and reinstate the NA::load_oop. AFAIU, this would be innocuous for other GCs, ZGC included.

Attention: @fisk.

Additional testing:

• Reproducer from the bug is no longer failing; used to fail 1/2 times on 3 platforms; does not fail after 50 tries
• Linux x86_64 fastdebug hotspot_gc_shenandoah
• Linux AArch64 fastdebug hotspot_gc_shenandoah

---

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-8306734: Shenandoah: Missing barriers on deoptimization path

Reviewers

• Erik Österlund (@fisk - Reviewer)
• Roman Kennke (@rkennke - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13613

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

• hotspot-runtime

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.

[shipilev]

/label add shenandoah

[openjdk]

@shipilev
The shenandoah label was successfully added.

[mlbridge]

Webrevs

• 02: Full - Incremental (e09cf682)
• 01: Full - Incremental (eec9493e)
• 00: Full (e515478b)

[fisk]

Ah. Apologies for removing your barriers - I forgot you about the safepoint between load and load barrier problem, requiring load barriers even on the stack. As mentioned in the description, avoiding these kinds of issues was indeed why we moved our barrier expansion to the assembly stage in ZGC. And indeed, that's why ZGC does not have any such barriers.
Unfortunately, the NativeAccess hack will not work for generational ZGC - it is going to crash immediately. So I preferred the more explicit and possibly less pretty conditional shenandoah code. What do you think?

[shipilev]

Ah. Apologies for removing your barriers - I forgot you about the safepoint between load and load barrier problem, requiring load barriers even on the stack. As mentioned in the description, avoiding these kinds of issues was indeed why we moved our barrier expansion to the assembly stage in ZGC. And indeed, that's why ZGC does not have any such barriers. Unfortunately, the NativeAccess hack will not work for generational ZGC - it is going to crash immediately. So I preferred the more explicit and possibly less pretty conditional shenandoah code. What do you think?

Thanks for the explanation. If the NativeAccess hack fails for ZGC, then I have to assume it might fail for other GCs too, and therefore using it, even though it uses a "generic" GC API, is still a hack. I reverted the last commit, so we now have the explicit barrier for Shenandoah there, pretty much like we used to have before Loom integration.

[fisk]

Looks good. Thanks for fixing.

[openjdk]

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

8306734: Shenandoah: Missing barriers on deoptimization path

Reviewed-by: eosterlund, rkennke

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

• a4a5385: 8306733: Remove template parameter of G1DetermineCompactionQueueClosure::free_pinned_region
• a98f587: 8306766: Reduce heap size for TestJNICriticalStressTest
• d53a5ee: 8306067: Open source AWT Graphics,GridBagLayout related tests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[rkennke]

Hmm ok, it's ugly but I don't have a better idea. Go for it!

[shipilev]

Testing is okay, the GHA failures are known.

/integrate

[openjdk]

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

• 9beae21: 8306718: Optimize and opensource some old AWT tests
• 36d61c3: 8306765: Some client related jtreg problem list entries are malformed
• 5f4965d: 8306687: Relax memory ordering constraints on metaspace atomic counters
• 33d6614: 8304845: Update PCSC-Lite for Suse Linux to 1.9.9 and fix incomplete license wording
• c92883a: 8306772: Remove sun.security.x509.CertException, sun.security.x509.CertParseError
• 0ff3a27: 8306008: Several Vector API tests fail for client VM after JDK-8304450
• e8f62de: 8304676: [vectorapi] x86_32: Crash in Assembler::kmovql(Address, KRegister)
• 98e8616: 8301169: java/net/httpclient/ThrowingSubscribersAsInputStream.java,ThrowingSubscribersAsInputStreamAsync.java, and other httpclient tests failing on windows: Unable to establish loopback connection
• 07ea445: 8306838: GetGraphicsTest needs to be headful
• 17ce097: 8266571: Sequenced Collections
• ... and 6 more: https://git.openjdk.org/jdk/compare/f968da97a5a5c68c28ad29d13fdfbe3a4adf5ef7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 28829f3.

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