RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v29]

[Patricio Chilano Mateo]

On Mon, 4 Nov 2024 20:55:07 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Update comment block in objectMonitor.cpp
>>  - Fix issue with unmounted virtual thread when dumping heap
>>  - Remove ThawBase::possibly_adjust_frame()
>
> 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_unmou...

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.

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

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