Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NOT TO MERGE YET] 8258604: Use 'isb' instruction in SpinPause on linux-aarch64 #5112

Closed
wants to merge 2 commits into from

Conversation

eastig
Copy link
Contributor

@eastig eastig commented Aug 13, 2021

To implement SpinPause an instruction creating a small delay without consuming ALU resources is needed. AArch64 has the YIELD instruction but most of AArch64 vendors still implement it as a NOP. It's been found that ISB can create a small delay without consuming ALU resources. It is more reliable than NOPs.

This patch implements SpinPause with ISB.
Testing: gtest, tier1, tier2

Results of https://github.com/ben-manes/caffeine/wiki/Benchmarks

GetPutBenchmark has 8 threads concurrently working with a cache.
GetPutBenchmark.read_only all 8 thread read from a cache.
GetPutBenchmark.write_only all 8 thread write to a cache.
GetPutBenchmark.readwrite 6 threads concurrently read from and 2 threads write to a cache.

Improvements when only 4 CPUs are available:

(cacheType)         Benchmark                               Units    Baseline       Error           CoV     New             Error           CoV     New vs Base
Cache2k             GetPutBenchmark.readwrite:readwrite_get ops/s   58,893,980.32   1,903,084.75    3.23%   65,685,338.20   4,858,329.69    7.40%   11.5%
ExpiringMap_Fifo    GetPutBenchmark.read_only               ops/s   5,060,227.98    695,582.74      13.75%  6,040,948.02    1,046,581.70    17.32%  19.4%
ExpiringMap_Fifo    GetPutBenchmark.readwrite:readwrite_put ops/s   433,046.21      194,297.64      44.87%  783,825.18      272,868.60      34.81%  81.0%
LinkedHashMap_Lru   GetPutBenchmark.read_only               ops/s   6,405,770.38    607,772.98      9.49%   10,259,858.51   1,446,471.70    14.10%  60.2%
LinkedHashMap_Lru   GetPutBenchmark.readwrite:readwrite_get ops/s   3,083,275.32    400,878.84      13.00%  5,934,784.52    1,309,784.39    22.07%  92.5%
LinkedHashMap_Lru   GetPutBenchmark.readwrite:readwrite_put ops/s   1,669,852.29    356,469.07      21.35%  2,491,807.50    299,230.23      12.01%  49.2%
LinkedHashMap_Lru   GetPutBenchmark.write_only              ops/s   4,623,049.93    449,438.94      9.72%   7,069,151.98    1,725,457.51    24.41%  52.9%

Improvements when only 8 CPUs are available:

(cacheType)         Benchmark                               Units    Baseline       Error       CoV     New             Error       CoV      New vs Base
LinkedHashMap_Lru   GetPutBenchmark.read_only               ops/s   4,054,507.30    71,693.81   1.77%   4,306,745.31    226,212.96  5.25%   6.2%
LinkedHashMap_Lru   GetPutBenchmark.readwrite:readwrite_get ops/s   3,087,578.38    86,158.78   2.79%   2,818,742.51    301,261.76  10.69%  -8.7%
LinkedHashMap_Lru   GetPutBenchmark.readwrite:readwrite_put ops/s   1,053,560.98    66,412.90   6.30%   1,401,928.05    239,055.34  17.05%  33.1%
LinkedHashMap_Lru   GetPutBenchmark.write_only              ops/s   3,823,208.44    305,096.58  7.98%   4,510,823.31    390,622.00  8.66%   18.0%

LinkedHashMapCache source
Cache2k source
ExpiringMapCache source
GetPutBenchmark source


Progress

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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5112

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 13, 2021

👋 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 openjdk bot added the rfr label Aug 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

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

@openjdk openjdk bot added the hotspot-runtime label Aug 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 13, 2021

Webrevs

Copy link
Member

@phohensee phohensee left a comment

SpinPause() should return 1 if it executes code. See the x86/64 implementations.

Fix: SpinPause() should return 1 if it executes code.
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

@eastig This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready label Aug 13, 2021
@eastig
Copy link
Contributor Author

@eastig eastig commented Aug 13, 2021

/integrate

@openjdk openjdk bot added the sponsor label Aug 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 10, 2021

@eastig This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

return 0;
// ISB creates a small delay without consuming ALU resources.
// This allows it to act as x86 PAUSE.
__asm volatile("isb");
Copy link
Contributor

@stooart-mon stooart-mon Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realised this was unconditional. Similar to my comment on the onSpinWait patch, while the ISB instruction happens to work for you just now on the platforms you have tested, there is no guarantee that it will continue to do so in future processors.
Could this be made optional? This is awkward, as it is C++ code, but it may have to be a build configuration option.

Copy link
Contributor Author

@eastig eastig Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Stuart,
Yes, I'll be updating it to support different implementations.

@eastig eastig changed the title 8258604: Use 'isb' instruction in SpinPause on linux-aarch64 [NOT TO MERGE YET] 8258604: Use 'isb' instruction in SpinPause on linux-aarch64 Oct 1, 2021
@openjdk openjdk bot removed sponsor ready rfr labels Oct 1, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 29, 2021

@eastig This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime
3 participants