8347489: RISC-V: Misaligned memory access with COH by RealFYang · Pull Request #23053 · openjdk/jdk

RealFYang · 2025-01-12T03:45:45Z

Hi, please consider this change.

We have different base_offset for T_BYTE/T_CHAR (4-byte instead of 8-byte aligned) with COH. This causes misaligned memory accesses for several instrinsics like String.Compare or String.Equals. The reason is that we assume 8-byte alignment and process one 8-byte word starting at the first array element for each iteration in the main loop. As a result, we have performance regressions on platforms with slow misaligned memory accesses like Unmatched and Premier P550 SBCs.

PS: Same issue is there even without COH. base_offset for T_BYTE/T_CHAR is 20 (thus 4-byte aligned) when UseCompressedClassPointers is disabled in this case.

Correctness test on linux-riscv64:

tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders") (release)
tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:-UseCompactObjectHeaders") (release)
hotspot:tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders") (fastdebug)
hotspot:tier1 (TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:-UseCompactObjectHeaders") (fastdebug)

Performance test on Premier P550 (-XX:+AlwaysPreTouch -Xms8g -Xmx8g):

SPECjbb2005:

1. Without Patch
1.1 -XX:+UseParallelGC -XX:-UseCompactObjectHeaders: 32666
1.2 -XX:+UseParallelGC -XX:+UseCompactObjectHeaders: 27610
1.3 -XX:+UseG1GC       -XX:-UseCompactObjectHeaders: 30911
1.4 -XX:+UseG1GC       -XX:+UseCompactObjectHeaders: 26008

2. With Patch
2.1 -XX:+UseParallelGC -XX:-UseCompactObjectHeaders: 32820
2.2 -XX:+UseParallelGC -XX:+UseCompactObjectHeaders: 34179
2.3 -XX:+UseG1GC       -XX:-UseCompactObjectHeaders: 30620
2.4 -XX:+UseG1GC       -XX:+UseCompactObjectHeaders: 31936

SPECjbb2015:

1. Without Patch
1.1 -XX:+UseG1GC -XX:-UseCompactObjectHeaders: max-jOPS = 1444, critical-jOPS = 431
1.2 -XX:+UseG1GC -XX:+UseCompactObjectHeaders: max-jOPS = 1092, critical-jOPS = 335

2. With Patch
2.1 -XX:+UseG1GC -XX:-UseCompactObjectHeaders: max-jOPS = 1452, critical-jOPS = 419
2.2 -XX:+UseG1GC -XX:+UseCompactObjectHeaders: max-jOPS = 1438, critical-jOPS = 477

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-8347489: RISC-V: Misaligned memory access with COH (Bug - P4)

Reviewers

Hamlin Li (@Hamlin-Li - Reviewer)
Vladimir Kempik (@VladimirKempik - Committer) Review applies to c093e451

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23053

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-12T03:47:08Z

👋 Welcome back fyang! 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-01-12T03:47:47Z

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

8347489: RISC-V: Misaligned memory access with COH

Reviewed-by: mli, vkempik

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

beae884: 8349150: Support precompiled headers on AIX
c545a3e: 8346774: Use Predicate classes instead of Node classes

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 · 2025-01-12T03:48:17Z

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

Hamlin-Li

Thanks for the patch.
Can you post the performance data in description?
And some minor comments.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

RealFYang · 2025-01-14T03:51:04Z

@Hamlin-Li
Thanks for the suggestions. I have updated accordingly. Note that base_offset is 12 and 16 bytes respectively w/wo COH.
And I have also added some specjbb2005 scores in PR description for reference.

mlbridge · 2025-01-14T03:54:46Z

Webrevs

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

Hamlin-Li · 2025-01-15T14:14:39Z

@Hamlin-Li Thanks for the suggestions. I have updated accordingly. Note that base_offset is 12 and 16 bytes respectively w/wo COH. And I have also added some specjbb2005 scores in PR description for reference.

Thank you posting the data!
As G1 is the default one, can you run the specjbb with G1? And it would be good to list both critial and max value for specjbb.
BTW, there are also some string comparison tests in jmh, e.g. StringCompareToDifferentLength and so on.

Hamlin-Li · 2025-01-15T14:20:21Z

-XX:+UseParallelGC -XX:+AlwaysPreTouch -Xms8g -Xmx8g

I guess the RVV is disabled by default? Will we also address the similar issue when RVV enabled?

Hamlin-Li

Some comments.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

RealFYang · 2025-01-16T10:51:53Z

@Hamlin-Li Thanks for the suggestions. I have updated accordingly. Note that base_offset is 12 and 16 bytes respectively w/wo COH. And I have also added some specjbb2005 scores in PR description for reference.

Thank you posting the data! As G1 is the default one, can you run the specjbb with G1? And it would be good to list both critial and max value for specjbb. BTW, there are also some string comparison tests in jmh, e.g. StringCompareToDifferentLength and so on.

The purpose of using ParallelGC for the test is to minimize the GC noise. I have also updated SPECjbb2005 score under G1GC in PR description for reference. Note that SPECjbb2005 score means bops (business operations per second). I will update SPECjbb2015 later which has max and critical jops. Thanks.

RealFYang · 2025-01-16T11:05:34Z

-XX:+UseParallelGC -XX:+AlwaysPreTouch -Xms8g -Xmx8g

I guess the RVV is disabled by default? Will we also address the similar issue when RVV enabled?

Yes. SBCs like Unmatched and P550 don't have RVV extensions. And I don't see such an issue with RVV code path for these intrinsics.

Hamlin-Li

Looks good, thanks for updating.
Just one minor comment.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

Hamlin-Li

Thank you!

RealFYang · 2025-02-04T14:01:04Z

Thanks all for the review!
/integrate

openjdk · 2025-02-04T14:03:07Z

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

d699aba: 8349135: Add tests for HttpRequest.Builder.copy()
81126c2: 8349238: Some more FFM benchmarks are broken
beae884: 8349150: Support precompiled headers on AIX
c545a3e: 8346774: Use Predicate classes instead of Node classes

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-04T14:03:16Z

@RealFYang Pushed as commit e91a6ec.

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

8347489: RISC-V: Misaligned memory access with COH

2ff64eb

openjdk bot added the hotspot-compiler [email protected] label Jan 12, 2025

Hamlin-Li reviewed Jan 13, 2025

View reviewed changes

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp Show resolved Hide resolved

RealFYang force-pushed the JDK-8347489 branch from d449fd1 to 2ff64eb Compare January 14, 2025 02:58

RealFYang added 2 commits January 14, 2025 11:18

Comment

fdce6f8

Add assertions

f9efaa3

RealFYang marked this pull request as ready for review January 14, 2025 03:48

openjdk bot added the rfr Pull request is ready for review label Jan 14, 2025

Fix assertions

23d9add

VladimirKempik reviewed Jan 14, 2025

View reviewed changes

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp Outdated Show resolved Hide resolved

Comment

caa2d48

VladimirKempik approved these changes Jan 15, 2025

View reviewed changes

Merge branch 'master' into JDK-8347489

c093e45

Hamlin-Li reviewed Jan 15, 2025

View reviewed changes

RealFYang added 4 commits January 27, 2025 11:47

Merge branch 'master' into JDK-8347489

5ba4e4e

Review comment

2292cc6

Review comment

b8c9916

Merge branch 'master' into JDK-8347489

0a47302

Hamlin-Li approved these changes Feb 3, 2025

View reviewed changes

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Feb 3, 2025

Merge branch 'master' into JDK-8347489

39e87c5

Review comment

db32665

RealFYang requested a review from Hamlin-Li February 4, 2025 07:02

openjdk bot removed the ready Pull request is ready to be integrated label Feb 4, 2025

Hamlin-Li approved these changes Feb 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 4, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 4, 2025

openjdk bot closed this Feb 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 4, 2025

Conversation

RealFYang commented Jan 12, 2025 • 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 Jan 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jan 12, 2025

Uh oh!

Hamlin-Li left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RealFYang commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Uh oh!

Hamlin-Li commented Jan 15, 2025

Uh oh!

Hamlin-Li commented Jan 15, 2025

Uh oh!

Hamlin-Li left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RealFYang commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RealFYang commented Jan 16, 2025

Uh oh!

Hamlin-Li left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hamlin-Li left a comment

Choose a reason for hiding this comment

Uh oh!

RealFYang commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

RealFYang commented Jan 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 12, 2025 •

edited

Loading

RealFYang commented Jan 14, 2025 •

edited

Loading

mlbridge bot commented Jan 14, 2025 •

edited

Loading

RealFYang commented Jan 16, 2025 •

edited

Loading