On Tue, 5 Nov 2024 23:50:29 GMT, Patricio Chilano Mateo
<pchilanom...@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
