8258192: Obsolete the CriticalJNINatives flag #6343

coleenp · 2021-11-10T22:06:05Z

This change removes the disabled CriticalJNINatives code, and the flag now gives an obsolete message.
Tested with tier1 on cpus x64, aarch64, and builds on linux-x86-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8258192: Obsolete the CriticalJNINatives flag

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to 9b8ff9a
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 9b8ff9a

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6343/head:pull/6343
$ git checkout pull/6343

Update a local copy of the PR:
$ git checkout pull/6343
$ git pull https://git.openjdk.java.net/jdk pull/6343/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6343

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6343.diff

bridgekeeper · 2021-11-10T22:06:35Z

👋 Welcome back coleenp! 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 · 2021-11-10T22:08:39Z

@coleenp 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 · 2021-11-10T22:12:43Z

Webrevs

shipilev · 2021-11-11T07:24:18Z

src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp

@@ -1823,7 +1743,7 @@ nmethod *SharedRuntime::generate_native_wrapper(MacroAssembler *masm,
  //
  // NW     [ABI_REG_ARGS]             <-- 1) R1_SP
  //        [outgoing arguments]       <-- 2) R1_SP + out_arg_slot_offset
-  //        [oopHandle area]           <-- 3) R1_SP + oop_handle_offset (save area for critical natives)
+  //        [oopHandle area]           <-- 3) R1_SP + oop_handle_offset (save area for critical natives) ?


?. The comment (save area for critical natives) must be redundant now.

I didn't know if the save area is still needed for something else, which is why I left the ?. I can remove the comment but haven't made any substantial changes here. I'm not sure if they're needed or not, but I can't test them if I made them.

I mean, you did the same here: https://github.com/openjdk/jdk/pull/6343/files#diff-060e534de775616a893aa969f3639e53666cda9e93bed7c3a3c14b9cdc4cdba0L1553-L1554 -- and that change is understandable.

Looking further, the area is needed (stores oops there), but not the comment.

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp

src/hotspot/share/runtime/sharedRuntime.cpp

TheRealMDoerr

Thanks for taking care of all platforms. move_ptr(MacroAssembler*, VMRegPair, VMRegPair, int) needs to get removed to avoid build warnings on PPC64 and s390.

coleenp

Thanks for reviewing, Aleksey. I made the changes and will retest.

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp

src/hotspot/share/runtime/sharedRuntime.cpp

coleenp · 2021-11-11T14:22:39Z

Thanks for finding move_ptr @TheRealMDoerr.

TheRealMDoerr

LGTM. Thanks!

openjdk · 2021-11-11T22:18:52Z

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

8258192: Obsolete the CriticalJNINatives flag

Reviewed-by: mdoerr, shade

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

coleenp · 2021-11-12T13:07:51Z

Thanks for reviewing @TheRealMDoerr . @shipilev does this look good now?

shipilev

Yeah, I am fine with this.

openjdk · 2021-11-12T14:06:23Z

@coleenp this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout critical
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

coleenp · 2021-11-12T17:02:45Z

Thanks @shipilev . All the GHA passed after resolving above merge conflict.
/integrate

openjdk · 2021-11-12T17:03:46Z

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

5a2452c: 8274835: Remove unnecessary castings in java.base
3b2585c: 8276658: Clean up JNI local handles code
aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-12T17:04:03Z

@coleenp Pushed as commit 0d2980c.

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

8258192: Obsolete the CriticalJNINatives flag

7e9c641

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 10, 2021

shipilev reviewed Nov 11, 2021

View changes

TheRealMDoerr suggested changes Nov 11, 2021

View changes

coleenp commented Nov 11, 2021

View changes

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp Show resolved Hide resolved

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

src/hotspot/share/runtime/sharedRuntime.cpp Show resolved Hide resolved

Some platform adjustments.

9b8ff9a

TheRealMDoerr approved these changes Nov 11, 2021

View changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 11, 2021

shipilev approved these changes Nov 12, 2021

View changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Nov 12, 2021

Merge master

525910a

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Nov 12, 2021

openjdk bot closed this Nov 12, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 12, 2021

coleenp deleted the critical branch Nov 12, 2021

8258192: Obsolete the CriticalJNINatives flag #6343

8258192: Obsolete the CriticalJNINatives flag #6343

coleenp commented Nov 10, 2021 •

edited by openjdk bot

bridgekeeper bot commented Nov 10, 2021

openjdk bot commented Nov 10, 2021

mlbridge bot commented Nov 10, 2021 •

edited

shipilev Nov 11, 2021

coleenp Nov 11, 2021

shipilev Nov 11, 2021

coleenp Nov 11, 2021

TheRealMDoerr left a comment

coleenp left a comment

coleenp commented Nov 11, 2021

TheRealMDoerr left a comment

openjdk bot commented Nov 11, 2021 •

edited

coleenp commented Nov 12, 2021

shipilev left a comment

openjdk bot commented Nov 12, 2021

coleenp commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

8258192: Obsolete the CriticalJNINatives flag #6343

8258192: Obsolete the CriticalJNINatives flag #6343

Conversation

coleenp commented Nov 10, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 10, 2021

openjdk bot commented Nov 10, 2021

mlbridge bot commented Nov 10, 2021 • edited

Webrevs

shipilev Nov 11, 2021

Choose a reason for hiding this comment

coleenp Nov 11, 2021

Choose a reason for hiding this comment

shipilev Nov 11, 2021

Choose a reason for hiding this comment

coleenp Nov 11, 2021

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

coleenp commented Nov 11, 2021

TheRealMDoerr left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 11, 2021 • edited

coleenp commented Nov 12, 2021

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 12, 2021

coleenp commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

coleenp commented Nov 10, 2021 •

edited by openjdk bot

mlbridge bot commented Nov 10, 2021 •

edited

openjdk bot commented Nov 11, 2021 •

edited