8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051 #13244

sunny868 · 2023-03-30T12:49:09Z

After JDK-8220051, Interpreter::notice_safepoints() only be executed at a safe point, so LoadLoad barrier is useless.
Barrier directives are generally time-consuming, so this patch removes LoadLoad be used for aarch64 and riscv.

Jtreg tier1-2 testing for aarch64 has been done.

Please help review it.

Thanks.

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-8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 9ef70f2c
Fei Yang (@RealFYang - Reviewer)
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13244

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-30T12:50:37Z

👋 Welcome back sunny868! 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 · 2023-03-30T12:52:51Z

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

mlbridge · 2023-03-30T12:56:09Z

Webrevs

TobiHartmann · 2023-04-03T05:28:50Z

/label remove hotspot-compiler

TobiHartmann · 2023-04-03T05:28:59Z

/label add hotspot-runtime

TobiHartmann · 2023-04-03T05:29:13Z

This should be reviewed by hotspot-runtime folks.

openjdk · 2023-04-03T05:29:32Z

@TobiHartmann
The hotspot-compiler label was successfully removed.

openjdk · 2023-04-03T05:30:36Z

@TobiHartmann
The hotspot-runtime label was successfully added.

sunny868 · 2023-04-03T06:29:14Z

This should be reviewed by hotspot-runtime folks.

Thanks @TobiHartmann

dholmes-ora

This really needs approval by someone involved in the Aarch64 and RiscV ports, but given notice_safepoints is now only used by JVMTI and called from a VM operation at a safepoint, I concur that these loadload barriers have no role to play in "seeing" the updated dispatch table.

Thanks.

dholmes-ora · 2023-04-04T02:23:01Z

May I suggest changing the PR and JBS issue title to:

8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051

openjdk · 2023-04-04T02:23:08Z

⚠️ @sunny868 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8305236
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-04-04T02:23:10Z

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

8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051

Reviewed-by: dholmes, fyang, 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 256 new commits pushed to the master branch:

9fb53ad: 8305716: Enhancements for printing age tables
ebba42a: 8305993: Add handleSocketErrorWithMessage to extend nio Net.c exception message
42b7260: 8306111: PPC64: RT call after thaw with exception requires larger ABI section
c7faf60: 8305757: Call Method::compute_has_loops_flag() when creating CDS archive
eb8d8cd: 8299129: Enhance NameService lookups
77df315: 8296692: Improve String platform support
f098b49: 8298310: Enhance TLS session negotiation
9e56d10: 8296832: Improve Swing platform support
2e5700a: 8288436: Improve Xalan supports
b1c34c0: 8296684: Improve String platform support
... and 246 more: https://git.openjdk.org/jdk/compare/7239150f8aff0e3dc07c5b27f6b7fb07237bfc55...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 (@dholmes-ora, @RealFYang, @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).

…ter JDK-8220051

openjdk · 2023-04-04T05:07:57Z

@sunny868 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

sunny868 · 2023-04-04T06:19:48Z

May I suggest changing the PR and JBS issue title to:

8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051

Done

sunny868 · 2023-04-04T06:25:46Z

Thanks @dholmes-ora to review. @theRealAph @RealFYang Please help confirm if there is still a problem with the patch.

RealFYang

That looks reasonable to me. I am performing some tests on linux-riscv64 platform.

sunny868 · 2023-04-04T07:37:53Z

That looks reasonable to me. I am performing some tests on linux-riscv64 platform.

Some StoreStore barriers is unnecessary also in interpreter. I had create a PR #13320, please review it together.

theaoqi · 2023-04-04T15:11:34Z

Please do the corresponding stress tests on LoongArch, @sunny868 .

RealFYang

Performed tier1-3 tests on linux-riscv64, result looks good. Also run jcstress test overnight.
Better to have an ACK from the aarch64 reviewers.

sunny868 · 2023-04-06T01:03:20Z

/integrate

openjdk · 2023-04-06T01:04:09Z

@sunny868
Your change (at version 73e2e91) is now ready to be sponsored by a Committer.

sunny868 · 2023-04-06T01:08:26Z

Please do the corresponding stress tests on LoongArch, @sunny868 .

The jcstress test runned OK on LoongArch.

sunny868 · 2023-04-13T02:30:11Z

@dholmes-ora can you sponsor it for me? thank you.

dholmes-ora · 2023-04-13T06:51:52Z

Better to have an ACK from the aarch64 reviewers.

I did not see such an ACK.

sunny868 · 2023-04-13T07:16:15Z

@theRealAph What is your opinion on this PR?

sunny868 · 2023-04-19T00:22:37Z

/label add aarch64

openjdk · 2023-04-19T00:23:32Z

@sunny868
The label aarch64 is not a valid label.
These labels are valid:

serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

theRealAph · 2023-04-19T07:45:53Z

This really needs approval by someone involved in the Aarch64 and RiscV ports, but given notice_safepoints is now only used by JVMTI and called from a VM operation at a safepoint, I concur that these loadload barriers have no role to play in "seeing" the updated dispatch table.

I don't think they ever had any such role to play, and I suspect they never have achieved anything.

RealFYang · 2023-04-21T00:45:42Z

/sponsor

openjdk · 2023-04-21T00:47:32Z

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

ffb2494: 8305207: Calendar.aggregateStamp(int, int) return value can be simplified
174c1a6: 4737887: (cal) API: Calendar methods taking field should document exceptions
f633623: 8306474: Move InstanceKlass read-only flags
afd2501: 8306482: Remove unused Method AccessFlags
d6cf4aa: 8305874: Open source AWT Key, Text Event related tests
9412c0a: 8297302: gtest/AsyncLogGtest.java fails AsyncLogTest.stdoutOutput_vm
20b1d19: 8305746: InitializeEncoding should cache Charset object instead of charset name
955abca: 8306483: (ch) Channels.newReader(ReadableByteChannel,Charset) refers to csName
c6a288d: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message
73018b3: 8306284: G1: Remove assertion in G1ScanHRForRegionClosure::do_claimed_block
... and 275 more: https://git.openjdk.org/jdk/compare/7239150f8aff0e3dc07c5b27f6b7fb07237bfc55...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-21T00:47:49Z

@RealFYang @sunny868 Pushed as commit 2c70828.

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

openjdk bot added the rfr Pull request is ready for review label Mar 30, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 30, 2023

openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 3, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Apr 3, 2023

dholmes-ora approved these changes Apr 4, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 4, 2023

8305236: Some LoadLoad barriers in the interpreter are unnecessary af…

73e2e91

…ter JDK-8220051

sunny868 force-pushed the 8305236 branch from 98d15f6 to 73e2e91 Compare April 4, 2023 05:07

sunny868 changed the title ~~8305236: The LoadLoad barrier has been useless after JDK-8220051~~ 8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051 Apr 4, 2023

RealFYang reviewed Apr 4, 2023

View reviewed changes

RealFYang approved these changes Apr 6, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 6, 2023

theRealAph approved these changes Apr 19, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 21, 2023

openjdk bot closed this Apr 21, 2023

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 Apr 21, 2023

8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051 #13244

8305236: Some LoadLoad barriers in the interpreter are unnecessary after JDK-8220051 #13244

Conversation

sunny868 commented Mar 30, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 30, 2023

Uh oh!

openjdk bot commented Mar 30, 2023

Uh oh!

mlbridge bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann commented Apr 3, 2023

Uh oh!

TobiHartmann commented Apr 3, 2023

Uh oh!

TobiHartmann commented Apr 3, 2023

Uh oh!

openjdk bot commented Apr 3, 2023

Uh oh!

openjdk bot commented Apr 3, 2023

Uh oh!

sunny868 commented Apr 3, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Apr 4, 2023

Uh oh!

openjdk bot commented Apr 4, 2023

Uh oh!

openjdk bot commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 4, 2023

Uh oh!

sunny868 commented Apr 4, 2023

Uh oh!

sunny868 commented Apr 4, 2023

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

sunny868 commented Apr 4, 2023

Uh oh!

theaoqi commented Apr 4, 2023

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

sunny868 commented Apr 6, 2023

Uh oh!

openjdk bot commented Apr 6, 2023

Uh oh!

sunny868 commented Apr 6, 2023

Uh oh!

sunny868 commented Apr 13, 2023

Uh oh!

dholmes-ora commented Apr 13, 2023

Uh oh!

sunny868 commented Apr 13, 2023

Uh oh!

sunny868 commented Apr 19, 2023

Uh oh!

openjdk bot commented Apr 19, 2023

Uh oh!

theRealAph commented Apr 19, 2023

Uh oh!

RealFYang commented Apr 21, 2023

Uh oh!

openjdk bot commented Apr 21, 2023

Uh oh!

openjdk bot commented Apr 21, 2023

sunny868 commented Mar 30, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 30, 2023 •

edited

Loading

openjdk bot commented Apr 4, 2023 •

edited

Loading