8358179: Performance regression in Math.cbrt #25962

missa-prime · 2025-06-24T22:33:56Z

The changes described below are meant to resolve the performance regression introduced by the x86_64 cbrt double precision floating point scalar intrinsic in #24470.

Check for +0, -0, +INF, -INF, and NaN before any other input values.
If these special values are found, return immediately with minimal modifications to the result register.
Performance testing shows the modified intrinsic improves throughput by 65.1% over the original intrinsic on average for the special values while throughput drops by 5.5% for the normal value range (-INF, -2^(-1022)], [2^(-1022), INF).

The commands to run all relevant micro-benchmarks are posted below.

make test TEST="micro:CbrtPerf.CbrtPerfRanges"
make test TEST="micro:CbrtPerf.CbrtPerfSpecialValues"

The results of all tests posted below were captured with an Intel® Xeon 8488C using OpenJDK v26-b1 as the baseline version. The term baseline1 refers to runs with the intrinsic enabled and baseline2 refers to runs with the intrinsic disabled.

Each result is the mean of 8 individual runs, and the input ranges used match those from the original Java implementation. Overall, the changes provide a significant uplift over baseline1 except for a mild regression in the (2^(-1022) <= |x| < INF) input range, which is expected due to the extra checks. When comparing against baseline2, the modified intrinsic significantly still outperforms for the inputs (-INF < x < INF) that require heavy compute. However, the special value inputs that trigger fast path returns still perform better with baseline2.

Input range(s)	Baseline1 (ops/ms)	Change (ops/ms)	Change vs baseline1 (%)
[-2^(-1022), 2^(-1022)]	18470	20847	+12.87
(-INF, -2^(-1022)], [2^(-1022), INF)	210538	198925	-5.52
[0]	344990	627561	+81.91
[-0]	291983	629941	+115.75
[INF]	382685	542211	+41.68
[-INF]	386174	542291	+40.43
[NaN]	421700	615157	+45.88

Input range(s)	Baseline2 (ops/ms)	Change (ops/ms)	Change vs baseline2 (%)
[-2^(-1022), 2^(-1022)]	7072	20847	+194.78
(-INF, -2^(-1022)], [2^(-1022), INF)	147884	198925	+34.51
[0]	1890520	627561	-66.80
[-0]	1890404	629941	-66.68
[INF]	1247633	542211	-56.54
[-INF]	1242287	542291	-56.35
[NaN]	1253700	615157	-50.93

Finally, the jtreg:test/jdk/java/lang/Math/CubeRootTests.java test passed with the changes.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8358179: Performance regression in Math.cbrt (Bug - P3)(⚠️ The fixVersion in this issue is [25] but the fixVersion in .jcheck/conf is 26, a new backport will be created when this pr is integrated.)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer)
Srinivas Vamsi Parasa (@vamsi-parasa - Author)
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25962/head:pull/25962
$ git checkout pull/25962

Update a local copy of the PR:
$ git checkout pull/25962
$ git pull https://git.openjdk.org/jdk.git pull/25962/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25962

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25962.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-24T22:34:49Z

👋 Welcome back missa! 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 · 2025-06-24T22:35:46Z

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

8358179: Performance regression in Math.cbrt

Reviewed-by: sviswanathan, sparasa, epeter

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

cd6caed: 8360783: CTW: Skip deoptimization between tiers
b32ccf2: 8361092: Remove trailing spaces in x86 ad files
0572b6e: 8360887: (fs) Files.getFileAttributeView returns unusable FileAttributeView if UserDefinedFileAttributeView unavailable (aix)
... and 210 more: https://git.openjdk.org/jdk/compare/8f733570040a7d7a24775e72244f47e946af191b...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 (@sviswa7, @eme64) 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).

openjdk · 2025-06-24T22:36:11Z

@missa-prime The following label will be automatically applied to this pull request:

hotspot-compiler

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.

…with andpd instruction

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp

mlbridge · 2025-06-25T18:41:40Z

Webrevs

01: Full - Incremental (615169d8)
00: Full (59201ed9)

missa-prime · 2025-06-25T21:12:20Z

@eme64 There seems to be an environment configuration issue (unrelated to code changes) on the Windows machine(s) this PR is landing on when running pre-submit tests. Could you run the internal tests on this PR to verify everything is ok?

eme64 · 2025-06-26T07:36:33Z

@missa-prime I just launched some testing. Yes, it seems the windows failures are unrelated - I've seen them on other PRs too.

eme64 · 2025-06-26T11:45:08Z

@missa-prime All tests passed in my internal testing.

eme64 · 2025-06-26T11:46:12Z

I'll hold off with approval until someone else who is more knowledgeable has reviewed first. But feel free to ping me for a second review.

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp

… enum for UCOMISD checks

sviswa7

Looks good to me.

missa-prime · 2025-06-27T15:53:06Z

I'll hold off with approval until someone else who is more knowledgeable has reviewed first. But feel free to ping me for a second review.

@eme64 Second review with the latest changes?

eme64 · 2025-06-30T05:51:58Z

@missa-prime The patch still looks good, though I ran testing again because of the new changes. Should complete in about 24h.

theRealAph · 2025-06-30T09:05:01Z

The changes described below are meant to resolve the performance regression introduced by the x86_64 cbrt double precision floating point scalar intrinsic in #24470.

Please add the performance for arguments in the normal range to this list.

missa-prime · 2025-06-30T15:32:32Z

The changes described below are meant to resolve the performance regression introduced by the x86_64 cbrt double precision floating point scalar intrinsic in #24470.

Please add the performance for arguments in the normal range to this list.

Sure, I added a line covering this.

vamsi-parasa

I did independent testing by running the correctness tests and performance benchmarks. The change looks good to me.

Thanks,
Vamsi

eme64

Did not review the patch in detail, but looks reasonable.

Tests are passing on my end with commit 3 / v01.

@missa-prime Thanks for taking care of this!

missa-prime · 2025-07-01T09:17:20Z

/integrate

openjdk · 2025-07-01T09:18:13Z

@missa-prime
Your change (at version 615169d) is now ready to be sponsored by a Committer.

sviswa7 · 2025-07-01T15:33:45Z

Thanks a lot @eme64.
/sponsor

openjdk · 2025-07-01T15:34:39Z

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

e1681c4: 8359477: com/sun/net/httpserver/Test12.java appears to have a temp file race
e138297: 8359436: AOTCompileEagerly should not be diagnostic
7583a7b: 8359337: XML/JAXP tests that make network connections should ensure that no proxy is selected
... and 218 more: https://git.openjdk.org/jdk/compare/8f733570040a7d7a24775e72244f47e946af191b...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-01T15:34:54Z

@sviswa7 @missa-prime Pushed as commit 38f59f8.

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

mur47x111 · 2025-07-02T11:25:33Z

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp

-  __ jmp(B1_4);
-
-  __ bind(L_2TAG_PACKET_6_0_1);
-  __ movsd(xmm0, ExternalAddress(NEG_INF), r11 /*rscratch*/);


note that NEG_INF is now unused

Got it - thanks.

Check for special values first in x86_64 cbrt intrinsic

4eaca6b

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 24, 2025

Make absolute mask memory constant 16 byte aligned for compatibility …

59201ed

…with andpd instruction

sendaoYan reviewed Jun 25, 2025

View reviewed changes

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp Outdated Show resolved Hide resolved

missa-prime marked this pull request as ready for review June 25, 2025 18:38

openjdk bot added the rfr Pull request is ready for review label Jun 25, 2025

sviswa7 reviewed Jun 26, 2025

View reviewed changes

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/x86/stubGenerator_x86_64_cbrt.cpp Outdated Show resolved Hide resolved

Ensure ABS_MASK is a 128-bit memory sized location and only use equal…

615169d

… enum for UCOMISD checks

sviswa7 approved these changes Jun 27, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 27, 2025

vamsi-parasa approved these changes Jun 30, 2025

View reviewed changes

eme64 approved these changes Jul 1, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 1, 2025

openjdk bot added the integrated Pull request has been integrated label Jul 1, 2025

openjdk bot closed this Jul 1, 2025

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 Jul 1, 2025

missa-prime deleted the user/missa-prime/cbrt branch July 1, 2025 15:43

mur47x111 reviewed Jul 2, 2025

View reviewed changes

8358179: Performance regression in Math.cbrt #25962

8358179: Performance regression in Math.cbrt #25962

Uh oh!

Conversation

missa-prime commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 24, 2025

Uh oh!

openjdk bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 24, 2025

Uh oh!

Uh oh!

mlbridge bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

missa-prime commented Jun 25, 2025

Uh oh!

eme64 commented Jun 26, 2025

Uh oh!

eme64 commented Jun 26, 2025

Uh oh!

eme64 commented Jun 26, 2025

Uh oh!

Uh oh!

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

missa-prime commented Jun 27, 2025

Uh oh!

eme64 commented Jun 30, 2025

Uh oh!

theRealAph commented Jun 30, 2025

Uh oh!

missa-prime commented Jun 30, 2025

Uh oh!

vamsi-parasa left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

missa-prime commented Jul 1, 2025

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

sviswa7 commented Jul 1, 2025

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

mur47x111 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

missa-prime Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

missa-prime commented Jun 24, 2025 •

edited

Loading

openjdk bot commented Jun 24, 2025 •

edited

Loading

mlbridge bot commented Jun 25, 2025 •

edited

Loading