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

8276054: JMH benchmarks for Fences #6138

Closed
wants to merge 4 commits into from

Conversation

shipilev
Copy link
Contributor

@shipilev shipilev commented Oct 27, 2021

While working on JDK-8252990, I realized there are no microbenchmarks for Fences. I would be good to add some. While technically we can benchmark Unsafe directly, I instead chose to use the public API, VarHandles, as this captures the real-world uses too.

Sample output for make test TEST=micro:vm.fences on x86_64:

Benchmark                      Mode  Cnt  Score    Error  Units
Multiple.acquire               avgt    9  0.408 ±  0.005  ns/op
Multiple.full                  avgt    9  4.694 ±  0.003  ns/op
Multiple.plain                 avgt    9  0.407 ±  0.004  ns/op
Multiple.release               avgt    9  0.407 ±  0.003  ns/op
Multiple.storeStore            avgt    9  0.409 ±  0.006  ns/op
MultipleWithLoads.acquire      avgt    9  4.600 ±  0.002  ns/op
MultipleWithLoads.full         avgt    9  8.322 ±  0.003  ns/op
MultipleWithLoads.plain        avgt    9  4.328 ±  0.001  ns/op
MultipleWithLoads.release      avgt    9  4.600 ±  0.002  ns/op
MultipleWithLoads.storeStore   avgt    9  4.600 ±  0.002  ns/op
MultipleWithStores.acquire     avgt    9  0.812 ±  0.001  ns/op
MultipleWithStores.full        avgt    9  5.291 ±  0.001  ns/op
MultipleWithStores.plain       avgt    9  0.812 ±  0.001  ns/op
MultipleWithStores.release     avgt    9  0.812 ±  0.001  ns/op
MultipleWithStores.storeStore  avgt    9  0.812 ±  0.001  ns/op
SafePublishing.plain           avgt    9  6.378 ±  0.011  ns/op
SafePublishing.release         avgt    9  6.368 ±  0.010  ns/op
SafePublishing.storeStore      avgt    9  6.376 ±  0.013  ns/op
Single.acquireFence            avgt    9  0.410 ±  0.009  ns/op
Single.fullFence               avgt    9  4.689 ±  0.002  ns/op
Single.plain                   avgt    9  0.409 ±  0.010  ns/op
Single.releaseFence            avgt    9  0.410 ±  0.006  ns/op
Single.storeStoreFence         avgt    9  0.410 ±  0.009  ns/op

Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6138

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 27, 2021

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

@openjdk openjdk bot commented Oct 27, 2021

@shipilev To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • build
  • client
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • ide-support
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Oct 27, 2021

/label hotspot

@openjdk openjdk bot added the hotspot label Oct 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2021

@shipilev
The hotspot label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 27, 2021

Webrevs

cl4es
cl4es approved these changes Oct 27, 2021
Copy link
Member

@cl4es cl4es left a comment

Benchmarks looks good.

I'm surprised the build handles omitting the org.openjdk.bench package prefix. I don't particularly mind either way (and maybe removing this prefix would be an overall win), but I think we should keep it consistent for now and use package org.openjdk.bench....

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2021

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

8276054: JMH benchmarks for Fences

Reviewed-by: redestad

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:

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.

@openjdk openjdk bot added the ready label Oct 27, 2021
@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Oct 27, 2021

I'm surprised the build handles omitting the org.openjdk.bench package prefix. I don't particularly mind either way (and maybe removing this prefix would be an overall win), but I think we should keep it consistent for now and use package org.openjdk.bench....

Ah yes, good spot. Fixed.

cl4es
cl4es approved these changes Oct 28, 2021
Copy link
Member

@cl4es cl4es left a comment

Thanks for making the package names consistent. New tests and restructuring looks good, too.

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Oct 28, 2021

Thanks! No more reviews needed for this, or?

@cl4es
Copy link
Member

@cl4es cl4es commented Oct 28, 2021

I would say you don't need more reviews for this.

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Oct 28, 2021

Okay then! Not sure what the rules for benchmarks are.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

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

  • 6d8fa8f: 8255286: Implement ParametersTypeData::print_data_on fully
  • 63b9f8c: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert.
  • cb989cf: 8275052: AArch64: Severe AES/GCM slowdown on MacOS for short blocks
  • c92f230: 8276110: Problemlist javax/swing/JMenu/4515762/bug4515762.java for macos12
  • 309acbf: 8275703: System.loadLibrary fails on Big Sur for libraries hidden from filesystem
  • abe52ae: 8275518: accessibility issue in Inet6Address docs
  • 85d8cd8: 8276100: Remove G1SegmentedArray constructor name parameter
  • a343fa8: 8275865: Print deoptimization statistics in product builds
  • bec977c: 8275917: Some locks shouldn't allow_vm_block
  • 7c996d5: 8269401: Merge "Exceptions" and "Errors" into "Exception Classes"
  • ... and 48 more: https://git.openjdk.java.net/jdk/compare/f6232982b91cb2314e96ddbde3984836a810a556...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 28, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

@shipilev Pushed as commit 5a768f7.

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

@cl4es
Copy link
Member

@cl4es cl4es commented Oct 28, 2021

I think the general rule in OpenJDK projects is simply one reviewer approval. I have not seen a need to try and add any special rules. I'm also not even sure where special rules - like those enforced by the hotspot maintainers (two reviewers, 24hr grace time) - are written down.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 28, 2021

The Hotspot rules are documented here:

https://wiki.openjdk.java.net/display/HotSpot/Pushing+a+HotSpot+change

and in the process of migrating to the OpenJDK dev guide.

@shipilev Not sure why this was even posted to hotspot-dev for review ??? :)

@cl4es
Copy link
Member

@cl4es cl4es commented Oct 28, 2021

@dholmes-ora we've not created a dedicated mailing list for microbenchmarks, rather stick to the convention that they are to be reviewed on the list(s) appropriate for the component being tested/stressed. Since these micros use VarHandle maybe they should have been reviewed on both core-libs and hotspot dev lists. Further then in this case I've interpreted the rules that even though the tests are VM-centric they don't touch the HotSpot source code and are thus not subject to the stricter set of rules put in place there.

@shipilev shipilev deleted the JDK-8276054-jmh-fences branch Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
3 participants