8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite() #10747

dean-long · 2022-10-18T17:37:30Z

This change adds a missing is_unloading() check for the callee in SharedRuntime::fixup_callers_callsite() and removes from_compiled_entry_no_trampoline() because it is no longer used.

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-8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite()

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)
Erik Österlund (@fisk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10747

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

Using diff file

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

bridgekeeper · 2022-10-18T17:39:52Z

👋 Welcome back dlong! 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 · 2022-10-18T17:43:29Z

@dean-long 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 · 2022-10-18T17:47:08Z

Webrevs

00: Full (b6adf7dd)

vnkozlov

Seems fine.

openjdk · 2022-10-18T18:44:20Z

@dean-long 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:

8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite()

Reviewed-by: kvn, thartmann, eosterlund

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

f5dabf9: 8295088: Update External Spec page to show tabs for hosts
2181042: 8295375: debug agent class tracking should not piggy back on the cbClassPrepare() callback
f41711e: 8295650: JFR: jfr scrub should warn if an event type doesn't exist
0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
15bebf9: 8295666: Linux x86 build fails after 8292591
5064718: 8294460: CodeSection::alignment checks for CodeBuffer::SECT_STUBS incorrectly
8b010e0: 8030616: sun/management/jmxremote/bootstrap/RmiBootstrapTest fails intermittently with cannot find a free port
b35922b: 8295714: GHA ::set-output is deprecated and will be removed
dfd2d83: 8295657: SA: Allow larger object alignments
a345df2: 8280131: jcmd reports "Module jdk.jfr not found." when "jdk.management.jfr" is missing
... and 56 more: https://git.openjdk.org/jdk/compare/5dbd49511518819acbbff9968cdf426af759cf2c...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dean-long · 2022-10-19T07:00:49Z

Thanks Vladimir.

TobiHartmann

Looks good to me.

dean-long · 2022-10-20T05:57:27Z

Thanks Tobias.

dean-long · 2022-10-20T05:58:41Z

@fisk, please take a look when you get the chance.

fisk

Looks good in general, but I have two questions.

fisk · 2022-10-20T07:35:14Z

src/hotspot/share/runtime/sharedRuntime.cpp

  CodeBlob* cb = CodeCache::find_blob(caller_pc);
-  if (cb == NULL || !cb->is_compiled() || entry_point == moop->get_c2i_entry()) {
+  if (cb == NULL || !cb->is_compiled() || callee->is_unloading()) {


Why not move the is_unloading check on callee to the if statement just above that checks the callee (as opposed to the callsite)?

I guess I was thinking is_unloading() can be a bit expensive the first time it is called, so it might be better to fail for other reasons first. But I believe is_unloading will eventually be called for every nmethod each unloading cycle, so avoiding the cost here just means moving it to somewhere else. I can move it to where you suggest if you like.

Okay I see. I'll leave it to you to decide if you prefer to move it or not. I'm okay with it either way.

fisk · 2022-10-20T07:38:00Z

src/hotspot/share/runtime/sharedRuntime.cpp

+  // Result from nmethod::is_unloading is not stable across safepoints.
+  NoSafepointVerifier nsv;
+
+  CompiledMethod* callee = moop->code();


There is a moop->code() null check just a few lines below, so now it looks like we are reading the code pointer twice checking if it is null. Is ot enough to do that one time?

It's actually the same number of null checks as before, if you look at what from_compiled_entry_no_trampoline() used to do. But I did consider removing the 2nd check, because no matter how late we check, we can always lose the race where it becomes null right after our last check. It's harmless however, so I decided to keep it.

Okay. That seems fine.

fisk

Looks good.

dean-long · 2022-10-22T02:09:59Z

Thanks Erik.

dean-long · 2022-10-22T02:10:06Z

/integrate

openjdk · 2022-10-22T02:12:08Z

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

6acbdb5: 8295427: popframe004: report more details on error
902162c: 8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher
f5dabf9: 8295088: Update External Spec page to show tabs for hosts
2181042: 8295375: debug agent class tracking should not piggy back on the cbClassPrepare() callback
f41711e: 8295650: JFR: jfr scrub should warn if an event type doesn't exist
0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
15bebf9: 8295666: Linux x86 build fails after 8292591
5064718: 8294460: CodeSection::alignment checks for CodeBuffer::SECT_STUBS incorrectly
8b010e0: 8030616: sun/management/jmxremote/bootstrap/RmiBootstrapTest fails intermittently with cannot find a free port
b35922b: 8295714: GHA ::set-output is deprecated and will be removed
... and 58 more: https://git.openjdk.org/jdk/compare/5dbd49511518819acbbff9968cdf426af759cf2c...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-22T02:12:20Z

@dean-long Pushed as commit b5efa2a.

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

check callee->is_unloading in fixup_callers_callsite

b6adf7d

dean-long marked this pull request as ready for review October 18, 2022 17:37

openjdk bot added the rfr Pull request is ready for review label Oct 18, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 18, 2022

vnkozlov approved these changes Oct 18, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2022

TobiHartmann approved these changes Oct 20, 2022

View reviewed changes

fisk suggested changes Oct 20, 2022

View reviewed changes

fisk approved these changes Oct 21, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 22, 2022

openjdk bot closed this Oct 22, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 22, 2022

8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite() #10747

8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite() #10747

dean-long commented Oct 18, 2022 •

edited by openjdk bot

bridgekeeper bot commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

mlbridge bot commented Oct 18, 2022

vnkozlov left a comment

openjdk bot commented Oct 18, 2022 •

edited

dean-long commented Oct 19, 2022

TobiHartmann left a comment

dean-long commented Oct 20, 2022

dean-long commented Oct 20, 2022

fisk left a comment

fisk Oct 20, 2022

dean-long Oct 21, 2022

fisk Oct 21, 2022

fisk Oct 20, 2022

dean-long Oct 21, 2022

fisk Oct 21, 2022

fisk left a comment

dean-long commented Oct 22, 2022

dean-long commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite() #10747

8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite() #10747

Conversation

dean-long commented Oct 18, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

mlbridge bot commented Oct 18, 2022

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 18, 2022 • edited

dean-long commented Oct 19, 2022

TobiHartmann left a comment

Choose a reason for hiding this comment

dean-long commented Oct 20, 2022

dean-long commented Oct 20, 2022

fisk left a comment

Choose a reason for hiding this comment

fisk Oct 20, 2022

Choose a reason for hiding this comment

dean-long Oct 21, 2022

Choose a reason for hiding this comment

fisk Oct 21, 2022

Choose a reason for hiding this comment

fisk Oct 20, 2022

Choose a reason for hiding this comment

dean-long Oct 21, 2022

Choose a reason for hiding this comment

fisk Oct 21, 2022

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

dean-long commented Oct 22, 2022

dean-long commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

dean-long commented Oct 18, 2022 •

edited by openjdk bot

openjdk bot commented Oct 18, 2022 •

edited