-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support virtual threads in JVMTI Resume* and Suspend* functions #15903
Conversation
Encountering hangs; debugging on my end. @fengxue-IS Let me know if you see anything out-of-order in these changes. |
Are you seeing the hang on just suspend/resume or suspendAll/ResumeAll ? |
openj9/runtime/jvmti/suspendhelper.cpp Line 53 in ef69481
|
Few issues:
@tajila @fengxue-IS @gacholio If you have quick & easy solutions, please share them here. Also, let me know if I have misinterpreted anything. |
ba00d67
to
2ab1d00
Compare
The following tests have successfully passed:
Note: Testing had to be run with The following tests could not be run successfully since some of the functions haven't been implemented:
|
@keithc-ca All of your feedback has been addressed. Also, removed the draft state. Can you please complete your review? |
2ab1d00
to
e452921
Compare
@keithc-ca I have addressed all the comments in the last feedback. But I will have to rework my changes because a deadlock was found while running the non-public cert tests last night. The rework will include adding a new lock. One lock will explicitly manage the vThreadInspectCount, and the other lock will explicitly handle the vThreadList. None of the locks will be held if there is a halt on internalEnterVMFromJNI/internalExitVMToJNI during suspension. The purpose of using two locks is to avoid intermittent deadlocks and improve perf by reducing the critical section's size. |
I'll move this to the draft state until you've addressed that. |
Addresses comments from eclipse-openj9#15903. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
e452921
to
961c131
Compare
Can you please detail the issue with the state field? Boolean fields (hidden or not) are 32 bits (no field is currently ever smaller than 32 bits). |
I'm not sure your approach is correct - for normal threads, there is no distinction made between JVMTI or java suspending/resuming a thread. This makes the APIs completely undependable, but I suspect the new APIs are equally bad. You'll need to test/prove that JVMTI and java suspend for virtual threads either has the same issue or does not. |
Java resume and suspend throw UnsupportedOperationException if invoked on a virtual thread: https://download.java.net/java/early_access/loom/docs/api/java.base/java/lang/Thread.html#suspend(). The two approaches are incomparable.
Just wanted to confirm the syntax and macros to use for a boolean field. Since boolean fields are 32 bits, the below usage is the correct approach?
|
The macros are correct. You have not described the issue with the vthread state. So there is no java way to suspend/resume a vthread? |
|
Regarding the race condition above, won't that already occur if multiple threads are trying to get the stack trace, or is there synchronization to prevent this? |
Jack says that the compare-and-set in VirtualThread.tryGetStackTrace will guarantee only one thread is allowed. Other threads will fail the compare-and-set and will have to retry. |
311a9b0
to
b841178
Compare
I just need to go over in detail the interaction between the suspend flag in the vthread and the halt flag to see if there are any timing holes. |
Are only unmounted vthreads marked as suspended using the new field? |
What is the justification for clearing the vthread flag when setting the publicFlag? Wouldn't things be simpler if the vhtread flag was always correct? |
Yes. Only unmounted vthreads are marked as suspended using the new field. For mounted vthreads, the state is derived from the carrier thread's J9VMThread, and the new field is unused.
At this point, the vthread is successfully mounted. The vthread flag is cleared when setting the publicFlag to be consistent with how mounted and unmounted vthreads are currently marked as suspended. Setting the vthread flag for both mounted and unmounted vthreads will require an extra |
This PR impacts the following JVMTI functions: - [Resume|Suspend]Thread - [Resume|Suspend]ThreadList - [Resume|Suspend]AllVirtualThreads The above functions have been either implemented or updated as per the JVMTI doc: https://download.java.net/java/early_access/jdk19/docs/specs/jvmti.html Other changes: - Added boolean inspectingLiveVirtualThreadList to synchronize between [Resume|Suspend]AllVirtualThreads, JVM_VirtualThreadMountBegin-End and JVM_VirtualThreadUnmountBegin-End. - If a thread is suspended while waiting on the monitor, then the monitor is released after waking up and before invoking internalEnterVMFromJNI, where the thread will be halted until resumed. After resuming, the monitor is acquired again. This helps avoid deadlocks. - Added hidden field isSuspendedByJVMTI in VirtualThread to track if the thread was suspended in JVMTI. If isSuspendedByJVMTI is non-zero, then J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND is set in carrier J9VMThread's publicFlags while resetting isSuspendedByJVMTI to zero. During mount, this suspends the thread if the thread was unmounted while JVMTI suspended it. Related: eclipse-openj9#15760 Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
1748dcd
to
e0508fe
Compare
@fengxue-IS Any comments? |
jenkins test sanity win jdk8 |
jenkins test sanity,extended zlinux jdk19 |
nothing from me, I think everything have been address |
This PR impacts the following JVMTI functions:
The above functions have been either implemented or updated as per the
JVMTI doc:
https://download.java.net/java/early_access/jdk19/docs/specs/jvmti.html
Other changes:
[Resume|Suspend]AllVirtualThreads, JVM_VirtualThreadMountBegin-End and
JVM_VirtualThreadUnmountBegin-End.
monitor is released after waking up and before invoking
internalEnterVMFromJNI, where the thread will be halted until resumed.
After resuming, the monitor is acquired again. This helps avoid
deadlocks.
thread was suspended in JVMTI. If isSuspendedByJVMTI is non-zero, then
J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND is set in carrier J9VMThread's
publicFlags while resetting isSuspendedByJVMTI to zero. During mount,
this suspends the thread if the thread was unmounted while JVMTI
suspended it.
Related: #15760
Signed-off-by: Babneet Singh sbabneet@ca.ibm.com