On Mon, 28 Oct 2024 00:35:11 GMT, David Holmes <dhol...@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
