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

[Patricio Chilano Mateo]

On Mon, 28 Oct 2024 00:35:11 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This vthread was unmounted on the call to `Object.wait`. Now it is mounted and "running" again, and we need to check which case it is in: notified, interrupted or timed-out. "First time" means it is the first time it's running after the original unmount on `Object.wait`. This is because once we are on the monitor reentry phase, the virtual thread can be potentially unmounted and mounted many times until it successfully acquires the monitor. Not sure how to rewrite the comment to make it clearer.
>
> The first sentence is not a sentence. Is it supposed to be saying:
> 
> // The first time we run after being preempted on Object.wait()
> // we check if we were interrupted or the wait timed-out ...
> 
> ?

Yes, I fixed the wording.

>> So the value stored in _owner has to be ANONYMOUS_OWNER. We cannot store the BasicLock* in there as before since it can clash with some other thread's tid. We store it in the new field _stack_locker instead.
>
> Right I understand we can't store the BasicLock* directly in owner, but the naming of this method has me confused as to what it actually does. With the old version we have:
> 
> Before: owner = BasicLock* belonging to current
> After: owner = JavaThread* of current
> 
> with the new version we have:
> 
> Before: owner = ANONYMOUS_OWNER
> After: owner = tid of current
> 
> so "BasicLock" doesn't mean anything here any more. Isn't this just `set_owner_from_anonymous` ?

I see your point. I removed this method and had the only caller just call set_owner_from_anonymous() and set_stack_locker(nullptr). There was one other caller in ObjectMonitor::complete_exit() but it was actually not needed so I removed it. ObjectMonitor::complete_exit() is only called today on JavaThread exit to possibly unlock monitors acquired through JNI that where not unlocked.

>> It is, we still increment _waiters for the vthread case.
>
> Sorry the target of my comment was not clear.  `thread_of_waiter` looks suspicious - will JVMTI find the vthread from the JavaThread?

If the ObjectWaiter is associated with a vthread(we unmounted in `Object.wait`) we just return null. We'll skip it from JVMTI code.

>> Only when facing contention on this call. But once we have the monitor we don't.
>
> But if this is from JNI then we have at least one native frame on the stack making the JNI call, so we have to be pinned if we were to block on the monitor. ???

We will have the native wrapper frame at the top, but we still need to add some extra check to differentiate this `jni_enter()` case with respect to the case of facing contention on a synchronize native method, where we do allow to unmount (only when coming from the interpreter since the changes to support it where minimal). I used the NoPreemptMark here, but we could filter this case anywhere along the freeze path. Another option could be to check `thread->current_pending_monitor_is_from_java()` in the ObjectMonitor code before trying to preempt.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1819907304
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815697784
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1819834478
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1819907921