8261649: AArch64: Optimize LSE atomics in C++ code #2611

theRealAph · 2021-02-17T17:14:46Z

Now that we have support for LSE atomics in C++ HotSpot source, we can generate much better code for them. In particular, the sequence we generate for CMPXCHG with a full two-way barrier using two DMBs is way suboptimal.

This patch:

Moves memory barriers from the atomic_linux_aarch64 file into the stubs.
Rewrites the LSE versions of the stubs to be more efficient.
Fixes a race condition in stub generation.
Mostly leaves the pre-LSE stubs alone, except that I added a PRFM which according to kernel engineers improves performance.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8261649: AArch64: Optimize LSE atomics in C++ code

Reviewers

Andrew Dinn (@adinn - Reviewer) ⚠️ Review applies to 23df448

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2611/head:pull/2611
$ git checkout pull/2611

bridgekeeper · 2021-02-17T17:20:46Z

👋 Welcome back aph! 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 · 2021-02-17T17:21:46Z

@theRealAph
The hotspot label was successfully added.

mlbridge · 2021-02-17T17:24:57Z

Webrevs

adinn · 2021-02-17T17:48:06Z

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

+      exchange_val = c_rarg2;
+    __ mov(prev, compare_val);
+    __ lse_cas(prev, exchange_val, ptr, size, acquire, release, /*not_pair*/true);
+    if (acquire && release) {


These two flags are only ever passed as true,true or false,false. Does any other combination make sense? If not then should you not be using a single flag? or at least asserting (pro tem) that they are both equal?

Today HotSpot only really supports mo_conservative and mo_relaxed, but there are many places in HotSpot where release on its own would make sense; I think Aleksey recently found some. Having said that, it would be clearer here to expose mo_conservative as well. I'll do so.

Clearer now?

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S

adinn

Ok, this looks good enough to me.

openjdk · 2021-02-18T16:29:16Z

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

8261649: AArch64: Optimize LSE atomics in C++ code

Reviewed-by: adinn

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

0e9c5ae: 8075909: [TEST_BUG] The regression-swing case failed as it does not have the 'Open' button when select 'subdir' folder with NimbusLAF
e9f3aab: 8261912: Code IfNode::fold_compares_helper more defensively
fd098e7: 8261838: Shenandoah: reconsider heap region iterators memory ordering
f94a845: 8261600: NMT: Relax memory order for updating MemoryCounter and fix racy updating of peak values
1a7adc8: 8260416: Remove unused method ReferenceProcessor::is_mt_processing_set_up()
3a21e1d: 8260653: Unreachable nodes keep speculative types alive
b695c7e: 8261925: ProblemList com/sun/jdi/AfterThreadDeathTest.java on Linux
97e1657: 8261846: [JVMCI] c2v_iterateFrames can get out of sync with the StackFrameStream
b8fd614: 8261939: os::strdup_check_oom() should be used in os::same_files() in os_windows.cpp
5f30829: 8202750: Reduce the use of get_canonical_path() in CDS
... and 3 more: https://git.openjdk.java.net/jdk/compare/05301f5fd2b5f2357024a3d7bb788270861220a8...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.

theRealAph · 2021-02-18T16:51:16Z

Ok, this looks good enough to me.

I pulled the vm_version.cpp change, which I committed by mistake. I think it's the right thing to do, but it needs a separate discussion. OK?

shipilev

Passer-by comments...

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

adinn · 2021-02-18T17:40:33Z

The memory_order_conservative/relaxed change is better. still good to go.

theRealAph · 2021-02-19T10:52:07Z

/integrate

openjdk · 2021-02-19T10:53:01Z

@theRealAph Since your change was applied there have been 24 commits pushed to the master branch:

61820b7: 8259984: IGV: Crash when drawing control flow before GCM
7e2c909: 8260485: Simplify and unify handler vectors in Posix signal code
c99eeb0: 8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
5caf686: 8261644: NMT: Simplifications and cleanups
ed93bc9: 8196301: java/awt/print/PrinterJob/Margins.java times out
7e78c77: 8261905: Move implementation of OopStorage num_dead related functions
78cde64: 8261860: Crash caused by lambda proxy class loaded in Shutdown hook
c158413: 8261098: Add clhsdb "findsym" command
0c31d5b: 8261977: Fix comment for getPrefixed() in canonicalize_md.c
9cf4f90: 8261473: Shenandoah: Add breakpoint support
... and 14 more: https://git.openjdk.java.net/jdk/compare/05301f5fd2b5f2357024a3d7bb788270861220a8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1b0c36b.

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

theRealAph added 6 commits February 12, 2021 11:09

Intermediate

c91ff23

Committed

cacc1ea

Flush everything correctly

b565b12

Trailing membar.

cbd1311

Merge https://github.com/openjdk/jdk into JDK-8261650

c5081fd

Restore LSE CAS generation.

23df448

openjdk bot added rfr Pull request is ready for review hotspot hotspot-dev@openjdk.org labels Feb 17, 2021

adinn reviewed Feb 17, 2021

View reviewed changes

Make things slightly less confusing

68ff88d

adinn approved these changes Feb 18, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 18, 2021

Remove mistaken change.

6b33890

shipilev reviewed Feb 18, 2021

View reviewed changes

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

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

Switch statement layout.

83016c1

theRealAph closed this Feb 19, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8261649: AArch64: Optimize LSE atomics in C++ code #2611

8261649: AArch64: Optimize LSE atomics in C++ code #2611

theRealAph commented Feb 17, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 17, 2021

openjdk bot commented Feb 17, 2021

mlbridge bot commented Feb 17, 2021 •

edited

Loading

adinn Feb 17, 2021

theRealAph Feb 18, 2021 •

edited

Loading

theRealAph Feb 18, 2021

adinn left a comment

openjdk bot commented Feb 18, 2021 •

edited

Loading

theRealAph commented Feb 18, 2021

shipilev left a comment

adinn commented Feb 18, 2021

theRealAph commented Feb 19, 2021

openjdk bot commented Feb 19, 2021

8261649: AArch64: Optimize LSE atomics in C++ code #2611

8261649: AArch64: Optimize LSE atomics in C++ code #2611

Conversation

theRealAph commented Feb 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 17, 2021

openjdk bot commented Feb 17, 2021

mlbridge bot commented Feb 17, 2021 • edited Loading

Webrevs

adinn Feb 17, 2021

Choose a reason for hiding this comment

theRealAph Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

theRealAph Feb 18, 2021

Choose a reason for hiding this comment

adinn left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 18, 2021 • edited Loading

theRealAph commented Feb 18, 2021

shipilev left a comment

Choose a reason for hiding this comment

adinn commented Feb 18, 2021

theRealAph commented Feb 19, 2021

openjdk bot commented Feb 19, 2021

theRealAph commented Feb 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 17, 2021 •

edited

Loading

theRealAph Feb 18, 2021 •

edited

Loading

openjdk bot commented Feb 18, 2021 •

edited

Loading