RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning

[Patricio Chilano Mateo]

On Tue, 5 Nov 2024 23:50:29 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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(con...
>
> Yes, I see your idea to get rid of the pending unmount event code. Before commenting on that, note that we still need to check if the freeze failed to undo the transition, which would call for this RAII object that we currently have. So in line with your suggestion we could call `VTMS_vthread_mount()` in `~JvmtiUnmountBeginMark()` which would also do the right thing too. Something like this: https://github.com/pchilano/jdk/commit/1729b98f554469fedbbce52333eccea9d1c81514
> We can go this simplified route, but note that we would post unmount/mount events even if we never unmounted or remounted because freeze failed. It's true that that is how it currently works when unmounting from Java fails, so I guess it's not new behavior.
> Maybe we could go with this simplified code now and work on it later. I think the unmount event should be always posted at the end of the transition, in `JvmtiVTMSTransitionDisabler::VTMS_unmount_end()`. I know that at that point we have already switched identity to the carrier, but does the specs say the event has to be posted in the context of the vthread? If we can do that then we could keep the simplified version and avoid this extra unmount/mount events.

Regarding the pop_frame/early_ret/async_exception conditions, not checking for them after we started the transition would be an issue.
For pop_frame/early_ret checks, the problem is that if any of them are installed in `JvmtiUnmountBeginMark()` while trying to start the transition, and later the call to freeze succeeds, when returning to the interpreter (monitorenter case) we will incorrectly follow the JVMTI code [1], instead of going back to `call_VM_preemptable` to clear the stack from the copied frames. As for the asynchronous exception check, if it gets installed in `JvmtiUnmountBeginMark()` while trying to start the transition, the exception would be thrown in the carrier instead, very likely while executing the unmounting logic.
When unmounting from Java, although the race is also there when starting the VTMS transition as you mentioned, I think the end result will be different. For pop_frame/early_ret we will just bail out if trying to install them since the top frame will be a native method (`notifyJvmtiUnmount`). For the async exception, we would process it on return from `notifyJvmtiUnmount` which would still be done in the context of the vthread.

[1]  https://github.com/openjdk/jdk/blob/471f112bca715d04304cbe35c6ed63df8c7b7fee/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L1629

>> 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.
>
>>  Is this posted after the VirtualThreadMount extension event posted?
>>
> It's posted before. We post the mount event at the end of thaw only if we are able to acquire the monitor: https://github.com/openjdk/jdk/blob/124efa0a6b8d05909e10005f47f06357b2a73949/src/hotspot/share/runtime/continuationFreezeThaw.cpp#L1620

> The JvmtiExport::post_monitor_waited() is called at the line 1801.
> Does it post the MonitorWaited event for this virtual thread as well?
>
That's the path a virtual thread will take if pinned. This case is when we were able to unmount the vthread. It is the equivalent, where the vthread finished the wait part (notified, interrupted or timed-out case) and it's going to retry acquiring the monitor.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830222411
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830227475