On Thu, 24 Oct 2024 21:08:26 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> This is the implementation of JEP 491: Synchronize Virtual Threads without >> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for >> further details. >> >> In order to make the code review easier the changes have been split into the >> following initial 4 commits: >> >> - Changes to allow unmounting a virtual thread that is currently holding >> monitors. >> - Changes to allow unmounting a virtual thread blocked on synchronized >> trying to acquire the monitor. >> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` >> and its timed-wait variants. >> - Changes to tests, JFR pinned event, and other changes in the JDK libraries. >> >> The changes fix pinning issues for all 4 ports that currently implement >> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added >> recently and stand in its own commit after the initial ones. >> >> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default >> locking mode, (and `LM_MONITOR` which comes for free), but not when using >> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been >> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), >> with the intention to remove `LM_LEGACY` code in future releases. >> >> >> ## Summary of changes >> >> ### Unmount virtual thread while holding monitors >> >> As stated in the JEP, currently when a virtual thread enters a synchronized >> method or block, the JVM records the virtual thread's carrier platform >> thread as holding the monitor, not the virtual thread itself. This prevents >> the virtual thread from being unmounted from its carrier, as ownership >> information would otherwise go wrong. In order to fix this limitation we >> will do two things: >> >> - We copy the oops stored in the LockStack of the carrier to the stackChunk >> when freezing (and clear the LockStack). We copy the oops back to the >> LockStack of the next carrier when thawing for the first time (and clear >> them from the stackChunk). Note that we currently assume carriers don't hold >> monitors while mounting virtual threads. >> >> - For inflated monitors we now record the `java.lang.Thread.tid` of the >> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This >> allows us to tie the owner of the monitor to a `java.lang.Thread` instance, >> rather than to a JavaThread which is only created per platform thread. The >> tid is already a 64 bit field so we can ignore issues of the counter >> wrapping around. >> >> #### General notes about this part: >> >> - Since virtual th... > > Patricio Chilano Mateo has updated the pull request incrementally with four > additional commits since the last revision: > > - Rename set/has_owner_anonymous to set/has_anonymous_owner > - Fix comments in javaThread.hpp and Thread.java > - Rename nonce/nounce to seqNo in VirtualThread class > - Remove ObjectMonitor::set_owner_from_BasicLock() Next batch of comments ... src/hotspot/share/classfile/javaClasses.cpp line 2082: > 2080: } > 2081: > 2082: bool java_lang_VirtualThread::set_onWaitingList(oop vthread, OopHandle& > list_head) { Some comments here about the operation would be useful. The "waiting list" here is just a list of virtual threads that need unparking by the Unblocker thread - right? I'm struggling to understand how a thread can already be on this list? 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? 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()))) { src/hotspot/share/runtime/objectMonitor.cpp line 132: > 130: > 131: // > ----------------------------------------------------------------------------- > 132: // Theory of operations -- Monitors lists, thread residency, etc: This comment block needs updating now owner is not a JavaThread*, and to account for vthread usage 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. 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? src/hotspot/share/runtime/objectMonitor.cpp line 1673: > 1671: > 1672: ContinuationEntry* ce = current->last_continuation(); > 1673: if (interruptible && ce != nullptr && ce->is_virtual_thread()) { So IIUC this use of `interruptible` would be explained as follows: // Some calls to wait() occur in contexts that still have to pin a vthread to its carrier. // All such contexts perform non-interruptible waits, so by checking `interruptible` we know // this is a regular Object.wait call. src/hotspot/share/runtime/objectMonitor.cpp line 1698: > 1696: // on _WaitSetLock so it's not profitable to reduce the length of > the > 1697: // critical section. > 1698: Please restore the blank line, else it looks like the comment block pertains to the `wait_reenter_begin`, but it doesn't. 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. 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. src/hotspot/share/runtime/objectMonitor.hpp line 174: > 172: > 173: int64_t volatile _owner; // Either tid of owner, NO_OWNER, > ANONYMOUS_OWNER or DEFLATER_MARKER. > 174: volatile uint64_t _previous_owner_tid; // thread id of the previous > owner of the monitor Looks odd to have the current owner as `int64_t` but we save the previous owner as `uint64_t`. ?? 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? 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. 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. ?? src/hotspot/share/runtime/objectMonitor.inline.hpp line 110: > 108: } > 109: > 110: // Returns null if DEFLATER_MARKER is observed. Comment needs updating 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 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? src/hotspot/share/runtime/synchronizer.cpp line 1440: > 1438: } > 1439: > 1440: ObjectMonitor* ObjectSynchronizer::inflate_impl(JavaThread* > inflating_thread, oop object, const InflateCause cause) { `inflating_thread` doesn't sound right as it is always the current thread that is doing the inflating. The passed in thread may be a different thread trying to acquire the monitor ... perhaps `contending_thread`? 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 ------------- PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2393922768 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815838204 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815839094 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815840245 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815985700 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815998417 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816002660 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816009160 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816014286 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816017269 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816018848 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815956322 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816040287 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815959203 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815960013 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815967260 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815969101 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816043275 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816047142 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816041444
