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

Fri Oct 25 18:50:22 UTC 2024

On Fri, 25 Oct 2024 00:26:24 GMT, David Holmes <dholmes at openjdk.org> wrote:

> The "waiting list" here is just a list of virtual threads that need unparking by the Unblocker thread - right?
>
Yes.

> src/hotspot/share/classfile/javaClasses.cpp line 2086:
> 
>> 2084:   jboolean vthread_on_list = Atomic::load(addr);
>> 2085:   if (!vthread_on_list) {
>> 2086:     vthread_on_list = Atomic::cmpxchg(addr, (jboolean)JNI_FALSE, (jboolean)JNI_TRUE);
> 
> It is not clear who the racing participants are here. How can the same thread be being placed on the list from two different actions?

The same example mentioned above, with a different timing, could result in two threads trying to add the same virtual thread to the list at the same time.

> src/hotspot/share/code/nmethod.cpp line 711:
> 
>> 709:   // handle the case of an anchor explicitly set in continuation code that doesn't have a callee
>> 710:   JavaThread* thread = reg_map->thread();
>> 711:   if ((thread->has_last_Java_frame() && fr.sp() == thread->last_Java_sp()) JVMTI_ONLY(|| (method()->is_continuation_enter_intrinsic() && thread->on_monitor_waited_event()))) {
> 
> Suggestion:
> 
>   if ((thread->has_last_Java_frame() && fr.sp() == thread->last_Java_sp()) 
>       JVMTI_ONLY(|| (method()->is_continuation_enter_intrinsic() && thread->on_monitor_waited_event()))) {

Fixed.

> src/hotspot/share/runtime/objectMonitor.cpp line 1140:
> 
>> 1138: }
>> 1139: 
>> 1140: bool ObjectMonitor::resume_operation(JavaThread* current, ObjectWaiter* node, ContinuationWrapper& cont) {
> 
> Explanatory comment would be good - thanks.

Added comment.

> src/hotspot/share/runtime/objectMonitor.cpp line 1532:
> 
>> 1530:   } else if (java_lang_VirtualThread::set_onWaitingList(vthread, vthread_cxq_head())) {
>> 1531:     // Virtual thread case.
>> 1532:     Trigger->unpark();
> 
> So ignoring for the moment that I can't see how `set_onWaitingList` could return false here, the check is just an optimisation to reduce the number of unparks issued i.e. only unpark if the list has changed?

Right.

> src/hotspot/share/runtime/objectMonitor.cpp line 2028:
> 
>> 2026:   // First time we run after being preempted on Object.wait().
>> 2027:   // Check if we were interrupted or the wait timed-out, and in
>> 2028:   // that case remove ourselves from the _WaitSet queue.
> 
> I'm not sure how to interpret this comment block - is this really two sentences because the first is not actually a sentence. Also unclear what "run" and "First time" relate to.

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.

> src/hotspot/share/runtime/objectMonitor.cpp line 2054:
> 
>> 2052:   // Mark that we are at reenter so that we don't call this method again.
>> 2053:   node->_at_reenter = true;
>> 2054:   assert(!has_owner(current), "invariant");
> 
> The position of this assert seems odd as it seems to be something that should hold at entry to this method.

Ok, I moved it to the beginning of resume_operation.

> src/hotspot/share/runtime/objectMonitor.hpp line 207:
> 
>> 205: 
>> 206:   static void Initialize();
>> 207:   static void Initialize2();
> 
> Please add comment why this needs to be deferred - and till after what?

Added comment.

> src/hotspot/share/runtime/objectMonitor.hpp line 312:
> 
>> 310:   void      set_successor(JavaThread* thread);
>> 311:   void      set_successor(oop vthread);
>> 312:   void      clear_successor();
> 
> Needs descriptive comments, or at least a preceding comment explaining what a "successor" is.

Added comment.

> src/hotspot/share/runtime/objectMonitor.hpp line 349:
> 
>> 347:   ObjectWaiter* first_waiter()                                         { return _WaitSet; }
>> 348:   ObjectWaiter* next_waiter(ObjectWaiter* o)                           { return o->_next; }
>> 349:   JavaThread* thread_of_waiter(ObjectWaiter* o)                        { return o->_thread; }
> 
> This no longer looks correct if the waiter is a vthread. ??

It is, we still increment _waiters for the vthread case.

> src/hotspot/share/runtime/objectMonitor.inline.hpp line 110:
> 
>> 108: }
>> 109: 
>> 110: // Returns null if DEFLATER_MARKER is observed.
> 
> Comment needs updating

Updated.

> src/hotspot/share/runtime/objectMonitor.inline.hpp line 130:
> 
>> 128: // Returns true if owner field == DEFLATER_MARKER and false otherwise.
>> 129: // This accessor is called when we really need to know if the owner
>> 130: // field == DEFLATER_MARKER and any non-null value won't do the trick.
> 
> Comment needs updating

Updated. Removed the second sentence, seemed redundant.

> src/hotspot/share/runtime/synchronizer.cpp line 670:
> 
>> 668:   // Top native frames in the stack will not be seen if we attempt
>> 669:   // preemption, since we start walking from the last Java anchor.
>> 670:   NoPreemptMark npm(current);
> 
> Don't we still pin for JNI monitor usage?

Only when facing contention on this call. But once we have the monitor we don't.

> src/hotspot/share/runtime/synchronizer.hpp line 172:
> 
>> 170: 
>> 171:   // Iterate ObjectMonitors where the owner is thread; this does NOT include
>> 172:   // ObjectMonitors where owner is set to a stack lock address in thread.
> 
> Comment needs updating

Updated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817192967
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817195264
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817195487
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817196602
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817197017
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817200025
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817200202
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817200507
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817195731
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817195899
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817196260
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817196374
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817200860
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817200711