8258015: [JVMCI] JVMCI_lock shouldn't be held while initializing box classes #1730
Conversation
|
👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Tested with: |
src/hotspot/share/jvmci/jvmci.cpp
Outdated
| } | ||
| MutexLocker locker(JVMCI_lock); | ||
| void JVMCI::ensure_box_caches_initialized(Mutex* lock, TRAPS) { | ||
| MutexLocker locker(lock); | ||
| // Check again after locking |
There was a problem hiding this comment.
Shall we remove 'again' in the comment?
| @@ -945,6 +945,10 @@ JVMCI::CodeInstallResult CodeInstaller::initialize_buffer(CodeBuffer& buffer, bo | |||
| } | |||
| } | |||
| #endif | |||
| if (_has_auto_box) { | |||
| JavaThread* THREAD = JavaThread::current(); | |||
| JVMCI::ensure_box_caches_initialized(JVMCI_lock, CHECK_(JVMCI::ok)); | |||
There was a problem hiding this comment.
Why we can hold JVMCI_lock here.
Does it mean that we'll never initialize the box classes at this point?
There was a problem hiding this comment.
I'm not sure I understand your question - why would we not be able to hold JVMCI_lock here?
That said, based on @veresov 's earlier comment, I'm now inclined to remove the lock altogether. As he says, at worst we harmlessly execute the loop in ensure_box_caches_initialized a few extra times. I've pushed this change.
There was a problem hiding this comment.
I think we should not hold JVMCI_lock here, because:
If JVMCI_lock was acquired, then no_safepoint_verifier(new_owner, true) was called [1].
--> Then, thread->_no_safepoint_count was increased [2].
--> Then, the assert [3] would fail due to _no_safepoint_count > 0.
What do you think?
Thanks.
[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/mutex.cpp#L480
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/mutex.cpp#L446
[3] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/thread.cpp#L795
There was a problem hiding this comment.
Yes, you're right - a VM lock should not be held when leaving the VM to call Java code.
As stated above, I've removed the lock altogether now (91571c1).
dougxc
changed the title
|
@dougxc 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: 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 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@dougxc Since your change was applied there have been 6 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit d163c6f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR fixes a regression caused by JDK-8257917 by not locking JVMCI_lock when initializing the boxing cache classes.
It also reduces the calls to
JVMCI::ensure_box_caches_initializedto be at most one per JVMCI code installation.Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1730/head:pull/1730$ git checkout pull/1730