On Mon, 4 Nov 2024 18:18:23 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 three > additional commits since the last revision: > > - Update comment block in objectMonitor.cpp > - Fix issue with unmounted virtual thread when dumping heap > - Remove ThawBase::possibly_adjust_frame() src/hotspot/share/runtime/continuation.cpp line 134: > 132: return true; > 133: } > 134: #endif // INCLUDE_JVMTI Could you, please, consider the simplification below? #if INCLUDE_JVMTI // return true if started vthread unmount bool jvmti_unmount_begin(JavaThread* target) { assert(!target->is_in_any_VTMS_transition(), "must be"); // Don't preempt if there is a pending popframe or earlyret operation. This can // be installed in start_VTMS_transition() so we need to check it here. if (JvmtiExport::can_pop_frame() || JvmtiExport::can_force_early_return()) { JvmtiThreadState* state = target->jvmti_thread_state(); if (target->has_pending_popframe() || (state != nullptr && state->is_earlyret_pending())) { return false; } } // Don't preempt in case there is an async exception installed since // we would incorrectly throw it during the unmount logic in the carrier. if (target->has_async_exception_condition()) { return false; } if (JvmtiVTMSTransitionDisabler::VTMS_notify_jvmti_events()) { JvmtiVTMSTransitionDisabler::VTMS_vthread_unmount(target->vthread(), true); } else { target->set_is_in_VTMS_transition(true); // not need to call: java_lang_Thread::set_is_in_VTMS_transition(target->vthread(), true) } return false; } static bool is_vthread_safe_to_preempt_for_jvmti(JavaThread* target) { if (target->is_in_VTMS_transition()) { // We caught target at the end of a mount transition. return false; } return true; } #endif // INCLUDE_JVMTI ... static bool is_vthread_safe_to_preempt(JavaThread* target, oop vthread) { assert(java_lang_VirtualThread::is_instance(vthread), ""); if (java_lang_VirtualThread::state(vthread) != java_lang_VirtualThread::RUNNING) { // inside transition return false; } return JVMTI_ONLY(is_vthread_safe_to_preempt_for_jvmti(target)) NOT_JVMTI(true); } ... int Continuation::try_preempt(JavaThread* target, oop continuation) { verify_preempt_preconditions(target, continuation); if (LockingMode == LM_LEGACY) { return freeze_unsupported; } if (!is_safe_vthread_to_preempt(target, target->vthread())) { return freeze_pinned_native; } JVMTI_ONLY(if (!jvmti_unmount_begin(target)) return freeze_pinned_native;) int res = CAST_TO_FN_PTR(FreezeContFnT, freeze_preempt_entry())(target, target->last_Java_sp()); log_trace(continuations, preempt)("try_preempt: %d", res); return res; } The following won't be needed: target->set_pending_jvmti_unmount_event(true); jvmtiThreadState.cpp: + if (thread->pending_jvmti_unmount_event()) { + assert(java_lang_VirtualThread::is_preempted(JNIHandles::resolve(vthread)), "should be marked preempted"); + JvmtiExport::post_vthread_unmount(vthread); + thread->set_pending_jvmti_unmount_event(false); + } As we discussed before there can be the `has_async_exception_condition()` flag set after a VTMS unmount transition has been started. But there is always such a race in VTMS transitions and the flag has to be processed as usual. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828376585
