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

[Serguei Spitsyn]

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

>> 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

Thank you for the comment! I'm okay with your modified suggestion in general if there are no road blocks.

> but does the specs say the event has to be posted in the context of the vthread?

As Alan said below we do not have an official spec for this but still the events need to be posted in vthread context.

> For pop_frame/early_ret checks ...

The pop_frame/early_ret conditions are installed in handshakes with a context of `JvmtiVTMSTransitionDisabler`. As you noted the `JVMTI_ERROR_OPAQUE_FRAME` might be also returned by the JVMTI `FramePop` and `ForceEarlyReturn*` for some specific cases. So, it feels like it should not be a problem. I'm thinking if adding an assert at the VTMS transition end would help.

> Maybe we could go with this simplified code now and work on it later...

Whatever works better for you. An alternate approach could be to file an enhancement to simplify/refactor this.
It would be nice to fix a couple of nits though:
 - the call to `java_lang_Thread::set_is_in_VTMS_transition()`is not needed in `JvmtiUnmountBeginMark`
 - the function `is_vthread_safe_to_preempt()` does not need the `vthread` parameter

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

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