Fix to handle suspend/resume of virtual/carrier threads #16943
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
6 times, most recently
from
2362f58
to
ca18ba7
Compare
|
@dipak-bagadiya After verifying that sanity.openjdk and extended.openjdk pass, please change PR's state to "Ready for review". |
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
3 times, most recently
from
cb0afcb
to
cdb97e6
Compare
|
@babsingh I made some changes to the code and ran the build. I'll check the results for sanity. openjdk and extended openjdk, once passed, will modify PR. |
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
from
cdb97e6
to
63b7237
Compare
babsingh
requested changes
Mar 30, 2023
|
Also, please review the commit message in MS Word to fix typos, sentence structure and grammar issues. |
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
from
63b7237
to
22d11a7
Compare
|
@dipak-bagadiya The revised suspend, resume and get thread state logic is posted in #16689 (comment). |
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
2 times, most recently
from
1a19b45
to
57ec6f7
Compare
babsingh
requested changes
Apr 7, 2023
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
from
57ec6f7
to
c204919
Compare
babsingh
requested changes
Apr 12, 2023
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
from
c204919
to
b74a222
Compare
babsingh
requested changes
Apr 14, 2023
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
4 times, most recently
from
7a5b7b7
to
36b28c2
Compare
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
3 times, most recently
from
5ab4901
to
e8e22fc
Compare
|
jenkins build sanity,extended.openjdk zlinux,win jdk19,jdk20 |
|
jenkins test sanity,extended.openjdk zlinux,win jdk19,jdk20 |
|
jenkins test sanity win jdk8 |
|
Failures from the below issues are seen in the PR builds:
Further investigation is required for the below failure. The below failure happens in JDK19 and not in JDK20. Below are failures that look like the known ones, but I did not find any open issues for the same reason, and they seem unrelated to the changes in this PR.
|
JVMTI treats a virtual thread and its carrier thread as two separate threads. If JVMTI suspends a virtual thread, then its carrier thread should not be suspended and vice-versa. Similarly, if JVMTI resumes a virtual thread, then its carrier thread should not be resumed and vice-versa. In OpenJ9, currently, a mounted virtual thread and its carrier thread shared the same J9VMThread. The thread state is derived from the J9VMThread. Due to the sharing of the J9VMThread in the mounted state,if a mounted virtual thread is suspended, then the JVMTI functions will also reflect that its carrier thread is suspended. Currently, "isSuspendedByJVMTI" is the hidden field that holds the suspend status for the virtual thread only, now it made available for carrier thread too, i.e. the scope has changed from "java/lang/VirtualThread" to "java/lang/Thread" threads Also, it is renamed to "isSuspendedInternal" as this hidden field is not just specific to JVMTI. The following functions has changed to set/reset and verify "isSuspendedInternal" status for threads:- 1) SUSPEND THREAD:- suspendhelper.cpp:suspendThread: jvmtiThread.c:jvmtiSuspendResumeCallBack thread.cpp:Java_java_lang_Thread_suspendImpl ->In case of suspend the halt flag (J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is set if the thread is mounted, i.e. threadObject == targetThread->threadObject in J9VMThread's publicFlags also the hidden field "isSuspendedInternal" set to a non-zero value for the thread instance in all cases regardless of thread being mounted or unmounted. ->If any of the public flags are already set, then the relevant failure message is assigned. 2) RESUME THREAD:- jvmtiThread.c:resumeThread: jvmtiThread.c:jvmtiSuspendResumeCallBack thread.cpp:Java_java_lang_Thread_resumeImpl ->In case of resume the halt flag (J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is cleared if the thread is mounted, i.e. threadObject == targetThread->threadObject in J9VMThread's publicFlags also the hidden field "isSuspendedInternal" set to a zero value for the thread instance in all cases regardless of thread being mounted or unmounted. ->If any of the public flags are already set, then the relevant failure message is assigned. 3) GETSTATE:- jvmtiHelpers.c:getThreadState jvmtiHelpers.c:getVirtualThreadState ->The getThreadState function is changed in such a way that will verify the "isSuspendedInternal" filed to check the thread is suspend or not. The basic concept behind the new changes: [mounted + unmounted] set the "isSuspendedInternal" field [mounted] the halt flag is only modified if the thread object is stored in targetThread->threadObject [unmounted] Delay setting the halt flag until the thread mount. Related: eclipse-openj9#16689 Signed-off-by: Dipak Bagadiya [email protected]
dipak-bagadiya
force-pushed
the
suspend_resume_status
branch
from
e8e22fc
to
5a37cb1
Compare
|
jenkins test sanity alinux64 jdk20 |
|
jenkins compile win jdk8 |
babsingh
approved these changes
May 3, 2023
There was a problem hiding this comment.
Overall, the code changes LGTM. re #16943 (comment): In the earlier PR builds, mostly known failures are seen. The VM-Access assertion is seen on JDK19 and not on JDK20; this is because JDK19 is out-of-service and has become stale.
dipak-bagadiya
added a commit
to dipak-bagadiya/aqa-tests
that referenced
this pull request
May 3, 2023
Test had been fixed by eclipse-openj9/openj9#16943 Signed-off-by: Dipak Bagadiya [email protected]
dipak-bagadiya
added a commit
to dipak-bagadiya/aqa-tests
that referenced
this pull request
May 3, 2023
Test has been fixed by eclipse-openj9/openj9#16943. Signed-off-by: Dipak Bagadiya [email protected]
karianna
pushed a commit
to adoptium/aqa-tests
that referenced
this pull request
May 3, 2023
Test has been fixed by eclipse-openj9/openj9#16943. Signed-off-by: Dipak Bagadiya [email protected] Signed-off-by: Dipak Bagadiya [email protected]
babsingh
reviewed
May 4, 2023
| while (J9_ARE_ALL_BITS_SET(targetThread->publicFlags, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND | J9_PUBLIC_FLAGS_VM_ACCESS)) { | ||
| omrthread_monitor_wait(targetThread->publicFlagsMutex); | ||
| vmFuncs->setHaltFlag(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND); | ||
| } else { |
There was a problem hiding this comment.
During the review, I was looking at the code with white-space hidden. So, I missed that the else block is indented incorrectly. It needs to include the monitor exit and goto calls.
@dipak-bagadiya This might be the potential fix for the assertion seen in #17334.
fyi @pshipton, we will thoroughly test the code before merging again.
Revised code:
...
#if JAVA_SPEC_VERSION >= 19
J9OBJECT_U32_STORE(currentThread, receiverObject, vm->isSuspendedInternalOffset, 1);
if (receiverObject == targetThread->threadObject)
#endif /* JAVA_SPEC_VERSION >= 19 */
{
if (currentThread == targetThread) {
vmFuncs->setHaltFlag(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
} else {
vmFuncs->internalExitVMToJNI(currentThread);
omrthread_monitor_enter(targetThread->publicFlagsMutex);
VM_VMAccess::setHaltFlagForVMAccessRelease(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
if (VM_VMAccess::mustWaitForVMAccessRelease(targetThread)) {
while (J9_ARE_ALL_BITS_SET(targetThread->publicFlags, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND | J9_PUBLIC_FLAGS_VM_ACCESS)) {
omrthread_monitor_wait(targetThread->publicFlagsMutex);
}
}
omrthread_monitor_exit(targetThread->publicFlagsMutex);
goto vmAccessReleased;
}
}
...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JVMTI treats a virtual thread and its carrier thread as two separate threads.
If JVMTI suspends a virtual thread, then its carrier thread
should not be suspended and vice-versa. Similarly, if JVMTI resumes
a virtual thread, then its carrier thread should not be resumed and
vice-versa.
In OpenJ9, currently, a mounted virtual thread and its carrier thread
shared the same J9VMThread. The thread state is derived from the
J9VMThread. Due to the sharing of the J9VMThread in the mounted
state, if a mounted virtual thread is suspended, then the JVMTI
functions will also reflect that its carrier thread is suspended.
Currently, "isSuspendedByJVMTI" is the hidden field that holds
the suspend status for the virtual thread only, now it made
available for carrier thread too, i.e., the scope has changed
from "java/lang/VirtualThread" to "java/lang/Thread" threads
Also, it is renamed to "isSuspendedInternal" as this hidden field is
not just specific to JVMTI.
The following functions has changed to set/reset and verify
"isSuspendedInternal" status and halt flag for threads: -
1) SUSPEND THREAD:-
suspendhelper.cpp:suspendThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_suspendImpl
->In case of suspend the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is set if the thread is
mounted, i.e., threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a non-zero value for the thread instance in all cases
regardless of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.
2) RESUME THREAD:-
jvmtiThread.c:resumeThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_resumeImpl
->In case of resume the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is cleared if the thread
is mounted, i.e., threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a zero value for the thread instance in all cases regardless
of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.
3) GETSTATE:-
jvmtiHelpers.c:getThreadState
jvmtiHelpers.c:getVirtualThreadState
->The getThreadState function is changed in such a way that will
verify the "isSuspendedInternal" filed to check the thread is
suspend or not.
The basic concept behind the new changes:-
[mounted + unmounted] set the "isSuspendedInternal" field
[mounted] the halt flag is only modified if the thread object is
stored in targetThread->threadObject
[unmounted] Delay setting the halt flag until the thread mount.
Related: #16689
Signed-off-by: Dipak Bagadiya [email protected]