8278241: Implement JVM SpinPause on linux-aarch64 #6803

eastig · 2021-12-10T21:02:53Z

This JVM SpinPause uses a spin wait stub. The stub is generated based on the SpinWait description which is defined with OnSpinWaitInst/OnSpinWaitInstCount options. The SpinWait provides the description of the instruction and the instruction count.

The SpinWait description is also used for the _onSpinWait() intrinsic. We don't have use cases when we need different implementations for the _onSpinWait() intrinsic and JVM SpinPause.

Testing results for fastdebug and release builds:

gtest: Passed
tier1...tier4: Passed
hotspot/jtreg/runtime/Thread/TestSpinPause.java: Passed

JVM SpinPause is used for the synchronised statements and can benchmarked with org.openjdk.bench.vm.lang.LockUnlock.testContendedLock.

Benchmarking results (number of samples per an experiment: 150) for Graviton2 (Neoverse N1), 1 ISB instruction:


+-----------+-------------------+------------+-----------+-----------+----------+---------+
| CPU cores | Contended threads | Base ns/op |   Error   |    New    |  Error   |  Diff   |
+-----------+-------------------+------------+-----------+-----------+----------+---------+
|         8 |                64 |  10007.213 | ±910.911  |  8527.346 | ±377.242 | -14.79% |
|        16 |                64 |  10274.935 | ±880.568  |  8310.433 | ±326.845 | -19.12% |
|        32 |                64 |  12231.947 | ±1525.364 |  9205.941 | ±394.409 | -24.74% |
|        64 |                64 |    9929.49 | ±586.074  | 10488.695 | ±570.458 | 5.63%   |
|        64 |                32 |   5605.119 | ±629.340  |  5023.882 | ±230.639 | -10.37% |
|        64 |                16 |   2817.346 | ±263.696  |  2367.528 | ±94.158  | -15.97% |
|        64 |                 2 |    870.389 | ±530.579  |   464.395 | ±126.260 | -46.65% |
+-----------+-------------------+------------+-----------+-----------+----------+---------+

Progress

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

Issue

JDK-8278241: Implement JVM SpinPause on linux-aarch64

Reviewers

Andrew Haley (@theRealAph - Reviewer)
Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6803/head:pull/6803
$ git checkout pull/6803

Update a local copy of the PR:
$ git checkout pull/6803
$ git pull https://git.openjdk.java.net/jdk pull/6803/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6803

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6803.diff

bridgekeeper · 2021-12-10T21:04:22Z

👋 Welcome back eastig! 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.

eastig · 2021-12-10T21:07:13Z

@nick-arm @theRealAph @stooart-mon
Hi, could you have a look please?

openjdk · 2021-12-10T21:08:57Z

@eastig 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 · 2021-12-10T21:14:35Z

Webrevs

theRealAph · 2021-12-11T09:25:51Z

@nick-arm @theRealAph @stooart-mon Hi, could you have a look please?

This is way too complicated. I'd use MacroAssembler::spin_wait() to generate a stub and call it from SpinPause.

stooart-mon · 2021-12-13T12:32:53Z

src/hotspot/cpu/aarch64/spin_wait_aarch64.hpp


+  SpinWait(Inst inst = NONE, int count = 0, InstRunner inst_runner = run_none) :
+      _inst(inst), _count(count), _inst_runner(inst_runner)  {}


Wouldn't it make more sense to have _inst_runner initialized in the constructor based on the value of Inst inst?
You aren't differentiating between the two in get_spin_wait_desc anyway.

eastig · 2021-12-13T15:55:20Z

@nick-arm @theRealAph @stooart-mon Hi, could you have a look please?

This is way too complicated. I'd use MacroAssembler::spin_wait() to generate a stub and call it from SpinPause.

Hi @theRealAph,

Thank you for advice. It was a good exercise to learn how to write a stub generator.
I reimplemented SpinPause to use a generated stub.

stooart-mon · 2021-12-13T17:24:58Z

src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp

-    return 0;
+    if (VM_Version::spin_wait_desc().inst() == SpinWait::NONE) {
+      return 0;
+    }


Would it be safe and more efficient to test func for NULL?

theRealAph · 2021-12-14T10:20:59Z

src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp

+    spin_wait_func_ptr_t func = CAST_TO_FN_PTR(spin_wait_func_ptr_t, StubRoutines::aarch64::spin_wait());
+    if (func == nullptr) {
+      return 0;
+    }


It's better simply to give _spin_wait a default value that points to a ret instruction.

theRealAph · 2021-12-14T10:56:41Z

src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp

@@ -382,7 +382,13 @@ int os::extra_bang_size_in_bytes() {

 extern "C" {
  int SpinPause() {
-    return 0;
+    using spin_wait_func_ptr_t = void (*)();


I think you'll want ThreadWXEnable wx(WXExec, thread); for Apple here.

Sorry, why do we need it here? This is linux-aarch64 implementation of SpinPause.
I think it should be in SpinPause in os_cpu/bsd_aarch64/os_bsd_aarch64.cpp. Am I right?

You are indeed right. We can worry about Apple another day.

theRealAph · 2021-12-14T15:01:42Z

Two more changes. The first makes the code simpler, and the second makes it less fragile.

diff --git a/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp b/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp
index a8b2820bb62..e946f3be970 100644
--- a/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp
@@ -6403,9 +6403,7 @@ class StubGenerator: public StubCodeGenerator {
     StubCodeMark mark(this, "StubRoutines", "spin_wait");
     address start = __ pc();
 
-    if (VM_Version::spin_wait_desc().inst() != SpinWait::NONE) {
-      __ spin_wait();
-    }
+    __ spin_wait();
     __ ret(lr);
 
     return start;
diff --git a/src/hotspot/cpu/aarch64/stubRoutines_aarch64.cpp b/src/hotspot/cpu/aarch64/stubRoutines_aarch64.cpp
index bb1a3325cea..f7c27ea7380 100644
--- a/src/hotspot/cpu/aarch64/stubRoutines_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/stubRoutines_aarch64.cpp
@@ -57,7 +57,10 @@ address StubRoutines::aarch64::_string_indexof_linear_uu = NULL;
 address StubRoutines::aarch64::_string_indexof_linear_ul = NULL;
 address StubRoutines::aarch64::_large_byte_array_inflate = NULL;
 address StubRoutines::aarch64::_method_entry_barrier = NULL;
-address StubRoutines::aarch64::_spin_wait = NULL;
+
+static void spin_wait_nop() { }
+address StubRoutines::aarch64::_spin_wait = CAST_FROM_FN_PTR(address, spin_wait_nop);
+
 bool StubRoutines::aarch64::_completed = false;
 
 /**

eastig · 2021-12-14T15:58:35Z

Two more changes. The first makes the code simpler, and the second makes it less fragile.

Thank you! I forgot __ spin_wait generates nothing with XX:OnSpinWaitInst=none.

Done.

openjdk · 2021-12-14T16:03:07Z

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

8278241: Implement JVM SpinPause on linux-aarch64

Reviewed-by: aph, phh

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

fcebe65: 8278842: Parallel: Remove unused VerifyObjectStartArrayClosure::_old_gen
4851ad8: 8278548: G1: Remove unnecessary check in forward_to_block_containing_addr
1e3ae3b: 8202579: Revisit VM_Version and VM_Version_ext for overlap and consolidation
7adf7f3: 8278351: Add function to retrieve worker_id from any context
758fe9b: 8273904: debug agent ArrayTypeImp::newInstance() fails to send reply packet if there is an error
c442587: 8277619: AArch64: Incorrect parameter type in Advanced SIMD Copy assembler functions
46f99ac: 8244765: Undo exclusiveAccess.dirs changes for JDK-8220295 and see if there are still any testing issues
54c9a99: 8278643: CoreUtils.getCoreFileLocation() should print out the size of the core file found
068a450: 8278825: Unused variable for diagnostic in Resolve
2def7e9: 8278584: compiler/vectorapi/VectorMaskLoadStoreTest.java failed with "Error: ShouldNotReachHere()"
... and 152 more: https://git.openjdk.java.net/jdk/compare/0d938cedcaf55886058b94dc280a8f7181c79fdf...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.

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 (@theRealAph, @phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eastig · 2021-12-15T11:16:20Z

/integrate

openjdk · 2021-12-15T11:18:16Z

@eastig
Your change (at version 08e8ce4) is now ready to be sponsored by a Committer.

phohensee · 2021-12-15T16:55:48Z

/sponsor

openjdk · 2021-12-15T16:57:42Z

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

fcebe65: 8278842: Parallel: Remove unused VerifyObjectStartArrayClosure::_old_gen
4851ad8: 8278548: G1: Remove unnecessary check in forward_to_block_containing_addr
1e3ae3b: 8202579: Revisit VM_Version and VM_Version_ext for overlap and consolidation
7adf7f3: 8278351: Add function to retrieve worker_id from any context
758fe9b: 8273904: debug agent ArrayTypeImp::newInstance() fails to send reply packet if there is an error
c442587: 8277619: AArch64: Incorrect parameter type in Advanced SIMD Copy assembler functions
46f99ac: 8244765: Undo exclusiveAccess.dirs changes for JDK-8220295 and see if there are still any testing issues
54c9a99: 8278643: CoreUtils.getCoreFileLocation() should print out the size of the core file found
068a450: 8278825: Unused variable for diagnostic in Resolve
2def7e9: 8278584: compiler/vectorapi/VectorMaskLoadStoreTest.java failed with "Error: ShouldNotReachHere()"
... and 152 more: https://git.openjdk.java.net/jdk/compare/0d938cedcaf55886058b94dc280a8f7181c79fdf...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-15T16:57:55Z

@phohensee @eastig Pushed as commit bcb79fd.

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

8278241: Implement JVM SpinPause on linux-aarch64

901b590

openjdk bot added the rfr Pull request is ready for review label Dec 10, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 10, 2021

stooart-mon reviewed Dec 13, 2021

View reviewed changes

Reimplement JVM SpinPause with using stub code

7c9877e

stooart-mon reviewed Dec 13, 2021

View reviewed changes

Check if StubRoutines::aarch64::spin_wait() is not null

419508b

theRealAph reviewed Dec 14, 2021

View reviewed changes

RET only StubRoutines::aarch64::spin_wait() for SpinWait::NONE

53d1658

Remove redundant check and guarantee non-null spin_wait

08e8ce4

theRealAph approved these changes Dec 14, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 14, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 15, 2021

phohensee approved these changes Dec 15, 2021

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 15, 2021

openjdk bot closed this Dec 15, 2021

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 15, 2021

eastig mentioned this pull request Dec 13, 2023

8321371: SpinPause() not implemented for bsd_aarch64/macOS #16994

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8278241: Implement JVM SpinPause on linux-aarch64 #6803

8278241: Implement JVM SpinPause on linux-aarch64 #6803

eastig commented Dec 10, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 10, 2021

eastig commented Dec 10, 2021

openjdk bot commented Dec 10, 2021

mlbridge bot commented Dec 10, 2021 •

edited

Loading

theRealAph commented Dec 11, 2021

stooart-mon Dec 13, 2021

eastig commented Dec 13, 2021

stooart-mon Dec 13, 2021

eastig Dec 13, 2021

eastig Dec 13, 2021

theRealAph Dec 14, 2021

eastig Dec 14, 2021

theRealAph Dec 14, 2021

eastig Dec 14, 2021 •

edited

Loading

theRealAph Dec 14, 2021

theRealAph commented Dec 14, 2021

eastig commented Dec 14, 2021

openjdk bot commented Dec 14, 2021 •

edited

Loading

eastig commented Dec 15, 2021

openjdk bot commented Dec 15, 2021

phohensee commented Dec 15, 2021

openjdk bot commented Dec 15, 2021

openjdk bot commented Dec 15, 2021


		SpinWait(Inst inst = NONE, int count = 0, InstRunner inst_runner = run_none) :
		_inst(inst), _count(count), _inst_runner(inst_runner) {}

8278241: Implement JVM SpinPause on linux-aarch64 #6803

8278241: Implement JVM SpinPause on linux-aarch64 #6803

Conversation

eastig commented Dec 10, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 10, 2021

eastig commented Dec 10, 2021

openjdk bot commented Dec 10, 2021

mlbridge bot commented Dec 10, 2021 • edited Loading

Webrevs

theRealAph commented Dec 11, 2021

Choose a reason for hiding this comment

eastig commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eastig Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theRealAph commented Dec 14, 2021

eastig commented Dec 14, 2021

openjdk bot commented Dec 14, 2021 • edited Loading

eastig commented Dec 15, 2021

openjdk bot commented Dec 15, 2021

phohensee commented Dec 15, 2021

openjdk bot commented Dec 15, 2021

openjdk bot commented Dec 15, 2021

eastig commented Dec 10, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 10, 2021 •

edited

Loading

eastig Dec 14, 2021 •

edited

Loading

openjdk bot commented Dec 14, 2021 •

edited

Loading