8272861: Add a micro benchmark for vector api #5234

sviswa7 · 2021-08-23T23:18:28Z

This pull request adds a micro benchmark for Vector API.
The Black Scholes algorithm is implemented with and without Vector API.
We see about ~6x gain with Vector API for this micro benchmark using 256 bit vectors.

Progress

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

Issue

JDK-8272861: Add a micro benchmark for vector api

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to 5b4abbf

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5234

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

Using diff file

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

bridgekeeper · 2021-08-23T23:19:37Z

👋 Welcome back sviswanathan! 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-08-23T23:22:12Z

@sviswa7 The following label will be automatically applied to this pull request:

core-libs

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-08-23T23:58:44Z

Webrevs

nsjian · 2021-08-24T03:35:29Z

test/micro/org/openjdk/bench/jdk/incubator/vector/BlackScholes.java

+        scalar_black_scholes_kernel(0);
+    }
+
+    static final VectorSpecies<Float> fsp = FloatVector.SPECIES_256;


Can we use a more portable way instead of fixed size SPECIES_256?

shipilev

Some benchmark comments.

shipilev · 2021-08-24T10:08:39Z

test/micro/org/openjdk/bench/jdk/incubator/vector/BlackScholes.java

+    }
+
+    @Benchmark
+    public void scalar_black_scholes(Blackhole bh) {


So there is a Blackhole argument here, but what is actually blackholed? AFAICS, stores to call and put are roughly okay to avoid dead-code elimination on these paths.

shipilev · 2021-08-24T10:08:55Z

test/micro/org/openjdk/bench/jdk/incubator/vector/BlackScholes.java

+    static float Y = 0.2316419f;
+    static float A1 = 0.31938153f;
+    static float A2 = -0.356563782f;
+    static float A3 = 1.781477937f;
+    static float A4 = -1.821255978f;
+    static float A5 = 1.330274429f;
+    static float PI = (float)Math.PI;


Want to make these static final?

sviswa7 · 2021-08-24T20:47:23Z

@shipilev @nsjian Thanks a lot for the feedback. I have implemented your review comments.

sviswa7 · 2021-08-25T20:48:43Z

@PaulSandoz @ericcaspole Looking forward to your review and approval for this vector api micro benchmark.

PaulSandoz

Very nice, just one minor comment (no need for another review), thanks for contributing this.

PaulSandoz · 2021-08-26T15:13:40Z

test/micro/org/openjdk/bench/jdk/incubator/vector/BlackScholes.java

+
+
+    float randFloat(float low, float high) {
+       float val = rand.nextFloat()/Float.MAX_VALUE;


nextFloat returns a PSR between 0 and 1., so no need to divide by Float.MAX_VALUE ?

openjdk · 2021-08-26T15:15:40Z

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

8272861: Add a micro benchmark for vector api

Reviewed-by: psandoz

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

fe7d708: 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong
845e1ce: 8272983: G1 Add marking details to eager reclaim logging
c420530: 8272481: [macos] javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java fails
e43a907: 8271315: Redo: Nimbus JTree renderer properties persist across L&F changes
11c9fd8: 8272975: ParallelGC: add documentation to heap memory layout
b94fd32: 8272859: Javadoc external links should only have feature version number in URL
9166ba3: 8272973: Incorrect compile command used by TestIllegalArrayCopyBeforeInfiniteLoop
49b2789: 8262751: RenderPipelineState assertion error in J2DDemo
a3308af: 8272836: Limit run time for java/lang/invoke/LFCaching tests
c4c76e2: 8272811: Document the effects of building with _GNU_SOURCE in os_posix.hpp
... and 36 more: https://git.openjdk.java.net/jdk/compare/04a806ec86a388b8de31d42f904c4321beb69e14...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.

sviswa7 · 2021-08-30T16:47:08Z

/integrate

openjdk · 2021-08-30T16:48:47Z

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

7a01ba6: 8272093: Extract evacuation failure injection from G1CollectedHeap
98b9d98: 8272797: Mutex with rank safepoint_check_never imply allow_vm_block
f11e099: 8272651: G1 heap region info print order changed by JDK-8269914
fbffa54: 8270438: "Cores to use" output in configure is misleading
5185dbd: 8273098: Unnecessary Vector usage in java.naming
276b07b: 8271490: [ppc] [s390]: Crash in JavaThread::pd_get_top_frame_for_profiling
bb7aa1c: 8272161: Make evacuation failure data structures local to collection
9ede41b: 8229031: Exporting CLASSPATH from shell can result in build failures
16e8305: 8273059: Redundant Math.min call in Http2ClientImpl#getConnectionWindowSize
f55d5ab: 8272838: Move CriticalJNI tests out of tier1
... and 62 more: https://git.openjdk.java.net/jdk/compare/04a806ec86a388b8de31d42f904c4321beb69e14...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-30T16:49:09Z

@sviswa7 Pushed as commit 5aaa20f.

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

sviswa7 added 2 commits August 23, 2021 16:01

8272861: Add a micro benchmark for vector api

be7c701

whitespace

ca688fa

openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 23, 2021

sviswa7 marked this pull request as ready for review August 23, 2021 23:52

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2021

nsjian reviewed Aug 24, 2021

View reviewed changes

shipilev reviewed Aug 24, 2021

View reviewed changes

sviswa7 added 2 commits August 24, 2021 13:28

Implement review comments

f92994c

Make constants as static final

5b4abbf

PaulSandoz approved these changes Aug 26, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 26, 2021

No need to normalize nextFloat

df22def

openjdk bot closed this Aug 30, 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 Aug 30, 2021

sviswa7 deleted the vapimicro branch June 1, 2022 21:32



		float randFloat(float low, float high) {
		float val = rand.nextFloat()/Float.MAX_VALUE;

8272861: Add a micro benchmark for vector api #5234

8272861: Add a micro benchmark for vector api #5234

Uh oh!

Conversation

sviswa7 commented Aug 23, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 23, 2021

Uh oh!

openjdk bot commented Aug 23, 2021

Uh oh!

mlbridge bot commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

nsjian Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

shipilev Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented Aug 24, 2021

Uh oh!

sviswa7 commented Aug 25, 2021

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

PaulSandoz Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sviswa7 commented Aug 30, 2021

Uh oh!

openjdk bot commented Aug 30, 2021

Uh oh!

openjdk bot commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

sviswa7 commented Aug 23, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 23, 2021 •

edited

Loading

openjdk bot commented Aug 26, 2021 •

edited

Loading