RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
Serguei Spitsyn
sspitsyn at openjdk.org
Wed Nov 6 17:40:08 UTC 2024
On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:
> This is the implementation of JEP 491: Synchronize Virtual Threads without Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for further details.
>
> In order to make the code review easier the changes have been split into the following initial 4 commits:
>
> - Changes to allow unmounting a virtual thread that is currently holding monitors.
> - Changes to allow unmounting a virtual thread blocked on synchronized trying to acquire the monitor.
> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` and its timed-wait variants.
> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>
> The changes fix pinning issues for all 4 ports that currently implement continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added recently and stand in its own commit after the initial ones.
>
> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default locking mode, (and `LM_MONITOR` which comes for free), but not when using `LM_LEGACY` mode. Note that the `LockingMode` flag has already been deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), with the intention to remove `LM_LEGACY` code in future releases.
>
>
> ## Summary of changes
>
> ### Unmount virtual thread while holding monitors
>
> As stated in the JEP, currently when a virtual thread enters a synchronized method or block, the JVM records the virtual thread's carrier platform thread as holding the monitor, not the virtual thread itself. This prevents the virtual thread from being unmounted from its carrier, as ownership information would otherwise go wrong. In order to fix this limitation we will do two things:
>
> - We copy the oops stored in the LockStack of the carrier to the stackChunk when freezing (and clear the LockStack). We copy the oops back to the LockStack of the next carrier when thawing for the first time (and clear them from the stackChunk). Note that we currently assume carriers don't hold monitors while mounting virtual threads.
>
> - For inflated monitors we now record the `java.lang.Thread.tid` of the owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This allows us to tie the owner of the monitor to a `java.lang.Thread` instance, rather than to a JavaThread which is only created per platform thread. The tid is already a 64 bit field so we can ignore issues of the counter wrapping around.
>
> #### General notes about this part:
>
> - Since virtual threads don't need to worry about holding monitors anymo...
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1082:
> 1080: } else {
> 1081: assert(vthread != nullptr, "no vthread oop");
> 1082: oop oopCont = java_lang_VirtualThread::continuation(vthread);
Nit: The name `oopCont` does not match the HotSpot naming convention.
What about `cont_oop` or even better just `cont` as at the line 2550?
src/hotspot/share/prims/jvmtiExport.cpp line 1682:
> 1680:
> 1681: // On preemption JVMTI state rebinding has already happened so get it always directly from the oop.
> 1682: JvmtiThreadState *state = java_lang_Thread::jvmti_thread_state(JNIHandles::resolve(vthread));
I'm not sure this change is right. The `get_jvmti_thread_state()` has a role to lazily create a `JvmtiThreadState` if it was not created before. With this change the `JvmtiThreadState` creation can be missed if the `unmount` event is the first event encountered for this particular virtual thread. You probably remember that lazy creation of the `JvmtiThreadState`'s is an important optimization to avoid big performance overhead when a JVMTI agent is present.
src/hotspot/share/prims/jvmtiExport.cpp line 2879:
> 2877: JvmtiVTMSTransitionDisabler::start_VTMS_transition((jthread)vthread.raw_value(), /* is_mount */ true);
> 2878: current->rebind_to_jvmti_thread_state_of(current->threadObj());
> 2879: }
This function looks a little bit unusual.
I need to think about the consequences but do not see anything bad so far.
I'll look at the `ObjectMonitor` and `continuation` side updates to get more details on this.
src/hotspot/share/runtime/continuation.cpp line 88:
> 86: if (_target->has_async_exception_condition()) {
> 87: _failed = true;
> 88: }
Q: I wonder why the failed conditions are not checked before the `start_VTMS_transition()` call. At least, it'd be nice to add a comment about on this.
src/hotspot/share/runtime/continuation.cpp line 115:
> 113: if (jvmti_present) {
> 114: _target->rebind_to_jvmti_thread_state_of(_target->threadObj());
> 115: if (JvmtiExport::should_post_vthread_mount()) {
This has to be `JvmtiExport::should_post_vthread_unmount()` instead of `JvmtiExport::should_post_vthread_mount()`.
Also, it'd be nice to add a comment explaining why the event posting is postponed to the `unmount` end point.
src/hotspot/share/runtime/continuation.cpp line 134:
> 132: return true;
> 133: }
> 134: #endif // INCLUDE_JVMTI
Could you, please, consider the simplification below?
#if INCLUDE_JVMTI
// return true if started vthread unmount
bool jvmti_unmount_begin(JavaThread* target) {
assert(!target->is_in_any_VTMS_transition(), "must be");
// Don't preempt if there is a pending popframe or earlyret operation. This can
// be installed in start_VTMS_transition() so we need to check it here.
if (JvmtiExport::can_pop_frame() || JvmtiExport::can_force_early_return()) {
JvmtiThreadState* state = target->jvmti_thread_state();
if (target->has_pending_popframe() || (state != nullptr && state->is_earlyret_pending())) {
return false;
}
}
// Don't preempt in case there is an async exception installed since
// we would incorrectly throw it during the unmount logic in the carrier.
if (target->has_async_exception_condition()) {
return false;
}
if (JvmtiVTMSTransitionDisabler::VTMS_notify_jvmti_events()) {
JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(target->vthread(), true);
} else {
target->set_is_in_VTMS_transition(true);
// not need to call: java_lang_Thread::set_is_in_VTMS_transition(target->vthread(), true)
}
return false;
}
static bool is_vthread_safe_to_preempt_for_jvmti(JavaThread* target) {
if (target->is_in_VTMS_transition()) {
// We caught target at the end of a mount transition.
return false;
}
return true;
}
#endif // INCLUDE_JVMTI
...
static bool is_vthread_safe_to_preempt(JavaThread* target, oop vthread) {
assert(java_lang_VirtualThread::is_instance(vthread), "");
if (java_lang_VirtualThread::state(vthread) != java_lang_VirtualThread::RUNNING) { // inside transition
return false;
}
return JVMTI_ONLY(is_vthread_safe_to_preempt_for_jvmti(target)) NOT_JVMTI(true);
}
...
int Continuation::try_preempt(JavaThread* target, oop continuation) {
verify_preempt_preconditions(target, continuation);
if (LockingMode == LM_LEGACY) {
return freeze_unsupported;
}
if (!is_safe_vthread_to_preempt(target, target->vthread())) {
return freeze_pinned_native;
}
JVMTI_ONLY(if (!jvmti_unmount_begin(target)) return freeze_pinned_native;)
int res = CAST_TO_FN_PTR(FreezeContFnT, freeze_preempt_entry())(target, target->last_Java_sp());
log_trace(continuations, preempt)("try_preempt: %d", res);
return res;
}
The following won't be needed:
target->set_pending_jvmti_unmount_event(true);
jvmtiThreadState.cpp:
+ if (thread->pending_jvmti_unmount_event()) {
+ assert(java_lang_VirtualThread::is_preempted(JNIHandles::resolve(vthread)), "should be marked preempted");
+ JvmtiExport::post_vthread_unmount(vthread);
+ thread->set_pending_jvmti_unmount_event(false);
+ }
As we discussed before there can be the `has_async_exception_condition()` flag set after a VTMS unmount transition has been started. But there is always such a race in VTMS transitions and the flag has to be processed as usual.
src/hotspot/share/runtime/objectMonitor.cpp line 1643:
> 1641: // actual callee (see nmethod::preserve_callee_argument_oops()).
> 1642: ThreadOnMonitorWaitedEvent tmwe(current);
> 1643: JvmtiExport::vthread_post_monitor_waited(current, node->_monitor, timed_out);
We post a JVMTI `MonitorWaited` event here for a virtual thread.
A couple of questions on this:
- Q1: Is this posted after the `VirtualThreadMount` extension event posted?
Unfortunately, it is not easy to make this conclusion.
- Q2: The `JvmtiExport::post_monitor_waited()` is called at the line 1801.
Does it post the `MonitorWaited` event for this virtual thread as well?
If not, then it is not clear how posting for virtual thread is avoided.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1820012783
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1820052049
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1820062505
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822235309
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822224512
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828376585
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1829199889
More information about the core-libs-dev
mailing list