8369190: JavaFrameAnchor on AArch64 has unnecessary barriers and wrong store order in MacroAssembler

[jcking]

Remove unnecessary release barriers in JavaFrameAnchor::{copy,clear} and fix MacroAssembler::set_last_Java_frame to set sp last as expected by the profiler.

---

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-8369190: JavaFrameAnchor on AArch64 has unnecessary barriers and wrong store order in MacroAssembler (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27645

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@jcking This change is no longer ready for integration - check the PR body for details.

[openjdk]

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

• hotspot

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]

Webrevs

• 03: Full - Incremental (016769df)
• 02: Full - Incremental (253417f9)
• 01: Full - Incremental (a1414494)
• 00: Full (a0058eb4)

[theRealAph]

Thanks. Add a comment to the effect that we don't need a fence because the profiler always reads from the same thread, and we're done.

[jcking]

Thanks. Add a comment to the effect that we don't need a fence because the profiler always reads from the same thread, and we're done.

Done. Let me know if you had something else in mind where you wanted the comment.

[theRealAph]

Good.

Although in the end we don't need a fence, you prompted me to measure the difference between a releasing store (STLR) and a DMB ST; STR and to my surprise STLR is way faster, at least on Apple hardware.

[theRealAph]

You still need a compiler fence here.

[theRealAph]

Maybe not, if this is the same thread as readers. I don't know what it's for.

[jcking]

OrderAccess::release() is more than a compiler barrier on AArch64. I am guessing this was copied from x86 or one of the others where that same function is a compiler barrier? Should I move compiler_barrier from the random source files its in, into globalDefinitions, and use that?

[theRealAph]

Sorry, approval rescinded.

[dean-long]

I don't think copy() is ever called. Can we remove it?