8277137: Set OnSpinWaitInst/OnSpinWaitInstCount defaults to "isb"/1 for Arm Neoverse N1

[eastig]

One ISB implementation of Thread.OnSpinWait shows performance improvements on Graviton2 (Arm Neoverse N1 implementation), e.g. #5562 (comment).

Testing:

• make test TEST=gtest: Passed
• make run-test TEST=tier1: Passed
• make run-test TEST=tier2: Passed
• make run-test TEST=hotspot/jtreg/compiler/onSpinWait: Passed

---

Progress

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

Issue

• JDK-8277137: Set OnSpinWaitInst/OnSpinWaitInstCount defaults to "isb"/1 for Arm Neoverse N1

Reviewers

• Paul Hohensee (@phohensee - Reviewer)
• Andrew Haley (@theRealAph - Reviewer)
• Nick Gasson (@nick-arm - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6415

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

Using diff file

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

[bridgekeeper]

👋 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.

[openjdk]

@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]

Webrevs

• 02: Full - Incremental (a9edcca6)
• 01: Full - Incremental (56258906)
• 00: Full (b3b8a23e)

[phohensee]

I'd explicitly set OnSpinWaitInstCount because one has to go find the default value in another file to understand what's going to happen. So I'd add:

FLAG_SET_DEFAULT(OnSpinWaitInstCount, 1);

[eastig]

I'd explicitly set OnSpinWaitInstCount because one has to go find the default value in another file to understand what's going to happen. So I'd add:

FLAG_SET_DEFAULT(OnSpinWaitInstCount, 1);

Thank you for reviewing.
Done.

[theRealAph]

Did we establish that this is the right default for Neoverse N1? I know that we've found a benchmark where it's a win, bit I'm not sure that's the same thing. On the other hand, do we know of possible cases where ISB makes things worse?

[eastig]

Hi Andrew,
Thank you for reviewing.

Did we establish that this is the right default for Neoverse N1?

This is based on:

• MySql: https://bugs.mysql.com/bug.php?id=100664
• MongoDB: https://jira.mongodb.org/browse/WT-6872
• Netty: Use cpu_relax() implementation for aarch64 netty/netty#11677
• Customers' benchmarks and workloads.
• Experiments with two and three ISB instructions.

On the other hand, do we know of possible cases where ISB makes things worse?

Thread.onSpinWait makes things worse when synchronisation overhead is not on the critical path. It might not improve performance when there is thread contention. In this case it might not give CPU resources to another thread. This applies to both arm64 and x86_64.
For example, my x86 system:

Architecture:        x86_64
CPU op-mode(s):      32-bit, 64-bit
Byte Order:          Little Endian
CPU(s):              16
On-line CPU(s) list: 0-15
Thread(s) per core:  2
Core(s) per socket:  8
Socket(s):           1
NUMA node(s):        1
Vendor ID:           GenuineIntel
CPU family:          6
Model:               85
Model name:          Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
Stepping:            7
CPU MHz:             3097.588
BogoMIPS:            4999.99
Hypervisor vendor:   KVM
Virtualization type: full
L1d cache:           32K
L1i cache:           32K
L2 cache:            1024K
L3 cache:            36608K

Results of org.openjdk.bench.java.lang.ThreadOnSpinWaitSharedCounter with 4 threads running on 2 vCPUs:

• taskset -c 0-1 build/linux-x86_64-server-release/images/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_onSpinWait -jar build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 3 org.openjdk.bench.java.lang.ThreadOnSpinWaitSharedCounter

Benchmark                            (maxNum)  (threadCount)  Mode  Cnt   Score   Error  Units
ThreadOnSpinWaitSharedCounter.trial   1000000              4  avgt   15  45.317 ± 1.741  ms/op

• taskset -c 0-1 build/linux-x86_64-server-release/images/jdk/bin/java -jar build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 3 org.openjdk.bench.java.lang.ThreadOnSpinWaitSharedCounter

Benchmark                            (maxNum)  (threadCount)  Mode  Cnt   Score   Error  Units
ThreadOnSpinWaitSharedCounter.trial   1000000              4  avgt   15  55.530 ± 4.606  ms/op

X86 PAUSE based implementation causes 22.5% slowdown.

[theRealAph]

Hi,

Did we establish that this is the right default for Neoverse N1?

This is based on:

* MySql: https://bugs.mysql.com/bug.php?id=100664

* MongoDB: https://jira.mongodb.org/browse/WT-6872

* Netty: [Use cpu_relax() implementation for aarch64 netty/netty#11677](https://github.com/netty/netty/pull/11677)

* Customers' benchmarks and workloads.

* Experiments with two and three `ISB` instructions.

OK, I'll buy that.

[openjdk]

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

8277137: Set OnSpinWaitInst/OnSpinWaitInstCount defaults to "isb"/1 for Arm Neoverse N1

Reviewed-by: phh, aph, ngasson

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

• db55f92: 8277343: dynamicArchive/SharedArchiveFileOption.java failed: '-XX:+RecordDynamicDumpInfo is unsupported when a dynamic CDS archive is specified in -XX:SharedArchiveFile:' missing
• 2f4b540: 8276314: [JVMCI] check alignment of call displacement during code installation
• 9160743: 8276058: Some swing test fails on specific CI macos system
• 8193800: 8274179: AArch64: Support SVE operations with encodable immediates
• b8453eb: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales
• 231fb61: 8276970: Default charset for PrintWriter that wraps PrintStream
• 29e552c: 8272358: Some tests may fail when executed with other locales than the US
• ce4471f: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
• 45a60db: 8277045: G1: Remove unnecessary set_concurrency call in G1ConcurrentMark::weak_refs_work
• 6bb0462: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE
• ... and 42 more: https://git.openjdk.java.net/jdk/compare/02f79008828b3dcce3bd6492efaa43e99376c0c5...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 (@phohensee, @theRealAph, @nick-arm) 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]

@theRealAph Thank you.

[phohensee]

Lgtm.

[eastig]

@phohensee Thank you.

[eastig]

/integrate

[openjdk]

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

[simonis]

/sponsor

[openjdk]

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

• 2c06bca: 8266368: Inaccurate after_unwind hook in C2 exception handler
• 77cc508: 8277215: Remove redundancy in ReferenceProcessor constructor args
• 0a65e8b: 8276794: Change nested classes in java.desktop to static nested classes
• db55f92: 8277343: dynamicArchive/SharedArchiveFileOption.java failed: '-XX:+RecordDynamicDumpInfo is unsupported when a dynamic CDS archive is specified in -XX:SharedArchiveFile:' missing
• 2f4b540: 8276314: [JVMCI] check alignment of call displacement during code installation
• 9160743: 8276058: Some swing test fails on specific CI macos system
• 8193800: 8274179: AArch64: Support SVE operations with encodable immediates
• b8453eb: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales
• 231fb61: 8276970: Default charset for PrintWriter that wraps PrintStream
• 29e552c: 8272358: Some tests may fail when executed with other locales than the US
• ... and 45 more: https://git.openjdk.java.net/jdk/compare/02f79008828b3dcce3bd6492efaa43e99376c0c5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@simonis @eastig Pushed as commit 38345bd.

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