8320379: C2: Sort spilling/unspilling sequence for better ld/st merging into ldp/stp on AArch64

[pengxiaolong]

Backport JDK-8320379: C2: Sort spilling/unspilling sequence for better ld/st merging into ldp/stp on AArch64

Validation

Tested on AWS Graviton instance by running JTREG_KEYWORDS="\!headful & \!external-dep & \!printer" make test TEST=all, same the test for PR on jdk17, only saw few test failures in CAInterop.java and compiler/intrinsics.

• CAInterop tests are known intermittently failing tests, it should be fine to ignore.
• The test failures from compiler/intrinsics has error message "Option 'UseSHA3Intrinsics' is expected to have 'true' value Option 'UseSHA3Intrinsics' should be enabled by default", it seems a bug but not related to the backport, will follow up we need to create a bug.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8320379 needs maintainer approval

Issue

• JDK-8320379: C2: Sort spilling/unspilling sequence for better ld/st merging into ldp/stp on AArch64 (Enhancement - P4 - Approved)

Reviewers

• Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/665/head:pull/665
$ git checkout pull/665

Update a local copy of the PR:
$ git checkout pull/665
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/665/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 665

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/665.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back pengxiaolong! 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]

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

8320379: C2: Sort spilling/unspilling sequence for better ld/st merging into ldp/stp on AArch64

Reviewed-by: aph

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

• af6eddc: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
• 160ea22: 8307352: AARCH64: Improve itable_stub
• 4509c90: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
• 150e228: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
• 8cfc0ef: 8316240: Open source several add/remove MenuBar manual tests
• 9c6183d: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
• 51d37ba: 8307193: Several Swing jtreg tests use class.forName on L&F classes
• e716aae: 8331626: unsafe.cpp:162:38: runtime error in index_oop_from_field_offset_long - applying non-zero offset 4563897424 to null pointer
• 4deef75: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
• e7a7af0: 8332154: Memory leak in SynchronousQueue
• ... and 9 more: https://git.openjdk.org/jdk21u-dev/compare/8204bd74df7b215c88ffdc309d59f68112c7ff42...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 (@theRealAph) 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]

This backport pull request has now been updated with issue from the original commit.

[openjdk]

⚠️ @pengxiaolong This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[mlbridge]

Webrevs

• 01: Full - Incremental (ef11ef0a)
• 00: Full (bf8a0da8)

[theRealAph]

Did you submit a jdk22 backport request?

[shipilev]

Did you submit a jdk22 backport request?

This issue is fixed in 22 originally, so no JDK 22u backport is needed.

Aside: my read of https://mail.openjdk.org/pipermail/jdk-updates-dev/2024-June/033864.html is that 22u is effectively dead anyway.

[theRealAph]

Did you submit a jdk22 backport request?

This issue is fixed in 22 originally, so no JDK 22u backport is needed.

OK.

Aside: my read of https://mail.openjdk.org/pipermail/jdk-updates-dev/2024-June/033864.html is that 22u is effectively dead anyway.

Ah, right. I didn't see that.

This is a pretty minor performance tweak, so it's marginal whether it should be back ported. I'm not sure the churn is justified.

[shipilev]

This is a pretty minor performance tweak, so it's marginal whether it should be back ported.

True. That said, lots of these papercut issues enjoy massive multipliers with our large fleets, so we prefer to patch up these minor inefficiencies as we go. :)

[pengxiaolong]

Thanks @theRealAph and @shipilev for the review and discussion. We have very large AArch64 fleet in our prod, which magnifies the minor performance improvements.

[pengxiaolong]

/approval request Backport to jdk21 to improve performance on AArch64, I have verified the the code by running all the tests, few failures not related to this build(details in PR); Applies cleanly. Risk should be low, JDK-8320379 was merged over 6 months ago w/o known issue/bug so far.

[openjdk]

@pengxiaolong
8320379: The approval request has been created successfully.

[theRealAph]

Thanks @theRealAph and @shipilev for the review and discussion. We have very large AArch64 fleet in our prod, which magnifies the minor performance improvements.

I understand that, but wouldn't that apply to all minor performance improvements? And wouldn't the direct consequence of following that principle to its conclusion be that if anyone wanted a stable release, too bad: there isn't a stable release? I totally get why this low-risk fix is a good thing, but...

[shipilev]

I understand that, but wouldn't that apply to all minor performance improvements? And wouldn't the direct consequence of following that principle to its conclusion be that if anyone wanted a stable release, too bad: there isn't a stable release? I totally get why this low-risk fix is a good thing, but...

Well, we don't have to drive that principle to conclusion. There are always changes in releases, and I believe that JDK 21 is the most fluid one at the moment, while JDK 8 is the most rigid one. It does not mean we don't do JDK 8 changes at all, though, so even that rigidity is not absolute.

There is always cost-benefit calculation in every backport. The little cost for backporting, testing, and future tracking is already taken by @pengxiaolong. We reap a little benefit for it in exchange. We do this in release that is an enticing migration target for LTS-tracking projects, giving migration a little more edge. What's not to like?

[mlbridge]

Mailing list message from Andrew Haley on jdk-updates-dev:

On 6/7/24 18:09, Aleksey Shipilev wrote:

On Fri, 7 Jun 2024 16:45:53 GMT, Andrew Haley <aph at openjdk.org> wrote:

I understand that, but wouldn't that apply to all minor performance improvements? And wouldn't the direct consequence of following that principle to its conclusion be that if anyone wanted a stable release, too bad: there isn't a stable release? I totally get why this low-risk fix is a good thing, but...

Well, we don't have to drive that principle to conclusion. There are always changes in releases, and I believe that JDK 21 is the most fluid one at the moment, while JDK 8 is the most rigid one. It does not mean we don't do JDK 8 changes at all, though, so even that rigidity is not absolute.

OK, fine. I was just checking. The "thousand paper cuts" argument made me think of "a thousand backports." :-)

There is always cost-benefit calculation in every backport. The little cost for backporting, testing, and future tracking is already taken by @pengxiaolong. We reap a little benefit for it in exchange. We do this in release that is an enticing migration target for LTS-tracking projects, giving migration a little more edge. What's not to like?

So if there's a cost-benefit argument to be made, then feature backport PRs (as opposed to bug fixes) should at least make the argument. It is not obvious to me where the cost-benefit argument lands in this case.

Bug fixes are different, I think.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[shipilev]

So if there's a cost-benefit argument to be made, then feature backport PRs (as opposed to bug fixes) should at least make the argument. It is not obvious to me where the cost-benefit argument lands in this case.

Out of my own curiosity, did the following test to confirm my gut feelings about this patch:

% for I in `seq 1 5`; do build/linux-aarch64-server-release/images/jdk/bin/java -Xcomp -XX:-TieredCompilation -Xbatch -XX:ActiveProcessorCount=1  -XX:+CITime Hello.java 2>&1 | grep "nmethod code size"; done

# Before
  nmethod code size         :  3781016 bytes
  nmethod code size         :  3780984 bytes
  nmethod code size         :  3780984 bytes
  nmethod code size         :  3780984 bytes
  nmethod code size         :  3780984 bytes

# After
  nmethod code size         :  3775176 bytes
  nmethod code size         :  3775176 bytes
  nmethod code size         :  3775176 bytes
  nmethod code size         :  3775144 bytes
  nmethod code size         :  3775176 bytes

In other words, on this trivial workload, this saves us ~0.15% of code cache. If this holds for larger apps, this amounts at about 192K for 128M code cache. This looks like a benefit enough for the cost of this backport :) It would, of course, impact even higher when we miss ldp optimization somewhere near heavy spills on a hot path.

[theRealAph]

In other words, on this trivial workload, this saves us ~0.15% of code cache. If this holds for larger apps, this amounts at about 192K for 128M code cache. This looks like a benefit enough for the cost of this backport :) It would, of course, impact even higher when we miss ldp optimization somewhere near heavy spills on a hot path.

So this is the kind of argument that should have been made.

It's not quite obvious that it'll help a hot path, though. ARMv8.4 made a change that makes LDP and STP atomic in some cases (see "Changes to single-copy atomicity in Armv8.4"). While this is very useful, it is a behavioural change. I'm guessing that there won't be any regressions for spills and fills, because stack-local accesses are uncontended.

[shipilev]

In other words, on this trivial workload, this saves us ~0.15% of code cache. If this holds for larger apps, this amounts at about 192K for 128M code cache. This looks like a benefit enough for the cost of this backport :) It would, of course, impact even higher when we miss ldp optimization somewhere near heavy spills on a hot path.

So this is the kind of argument that should have been made.

True. Are you satisfied with it?

It's not quite obvious that it'll help a hot path, though. ARMv8.4 made a change that makes LDP and STP atomic in some cases (see "Changes to single-copy atomicity in Armv8.4"). While this is very useful, it is a behavioural change. I'm guessing that there won't be any regressions for spills and fills, because stack-local accesses are uncontended.

Yup. I expect that if LDP/STP merging becomes problematic on some platforms, the merging code would somehow account for that. This seems orthogonal to what this patch is doing.

[theRealAph]

In other words, on this trivial workload, this saves us ~0.15% of code cache. If this holds for larger apps, this amounts at about 192K for 128M code cache. This looks like a benefit enough for the cost of this backport :) It would, of course, impact even higher when we miss ldp optimization somewhere near heavy spills on a hot path.

So this is the kind of argument that should have been made.

True. Are you satisfied with it?

Yes, I think so. It's remarkably effective for such a simple optimization.

[pengxiaolong]

Thank you @theRealAph and @shipilev for the review/discussion/argument, it is a good learning! I"ll start to integrate.

[pengxiaolong]

/integrate

[openjdk]

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

[shipilev]

/sponsor

[openjdk]

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

• af6eddc: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
• 160ea22: 8307352: AARCH64: Improve itable_stub
• 4509c90: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
• 150e228: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
• 8cfc0ef: 8316240: Open source several add/remove MenuBar manual tests
• 9c6183d: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
• 51d37ba: 8307193: Several Swing jtreg tests use class.forName on L&F classes
• e716aae: 8331626: unsafe.cpp:162:38: runtime error in index_oop_from_field_offset_long - applying non-zero offset 4563897424 to null pointer
• 4deef75: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
• e7a7af0: 8332154: Memory leak in SynchronousQueue
• ... and 9 more: https://git.openjdk.org/jdk21u-dev/compare/8204bd74df7b215c88ffdc309d59f68112c7ff42...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @pengxiaolong Pushed as commit a86d042.

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