On Tue, 29 Oct 2024 02:56:30 GMT, Serguei Spitsyn <sspit...@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... > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1082: > >> 1080: } else { >> 1081: assert(vthread != nullptr, "no vthread oop"); >> 1082: oop oopCont = java_lang_VirtualThread::continuation(vthread); > > Nit: The name `oopCont` does not match the HotSpot naming convention. > What about `cont_oop` or even better just `cont` as at the line 2550? Renamed to cont. > src/hotspot/share/prims/jvmtiExport.cpp line 1682: > >> 1680: >> 1681: // On preemption JVMTI state rebinding has already happened so get >> it always directly from the oop. >> 1682: JvmtiThreadState *state = >> java_lang_Thread::jvmti_thread_state(JNIHandles::resolve(vthread)); > > I'm not sure this change is right. The `get_jvmti_thread_state()` has a role > to lazily create a `JvmtiThreadState` if it was not created before. With this > change the `JvmtiThreadState` creation can be missed if the `unmount` event > is the first event encountered for this particular virtual thread. You > probably remember that lazy creation of the `JvmtiThreadState`'s is an > important optimization to avoid big performance overhead when a JVMTI agent > is present. Right, good find. I missed `get_jvmti_thread_state ` will also create the state if null. How about this fix: https://github.com/pchilano/jdk/commit/baf30d92f79cc084824b207a199672f5b7f9be88 I now also see that JvmtiVirtualThreadEventMark tries to save some state of the JvmtiThreadState for the current thread before the callback, which is not the JvmtiThreadState of the vthread for this case. Don't know if something needs to change there too. > src/hotspot/share/runtime/continuation.cpp line 88: > >> 86: if (_target->has_async_exception_condition()) { >> 87: _failed = true; >> 88: } > > Q: I wonder why the failed conditions are not checked before the > `start_VTMS_transition()` call. At least, it'd be nice to add a comment about > on this. These will be rare conditions so I don't think it matters to check them before. But I can move them to some method that we call before and after if you prefer. > src/hotspot/share/runtime/continuation.cpp line 115: > >> 113: if (jvmti_present) { >> 114: _target->rebind_to_jvmti_thread_state_of(_target->threadObj()); >> 115: if (JvmtiExport::should_post_vthread_mount()) { > > This has to be `JvmtiExport::should_post_vthread_unmount()` instead of > `JvmtiExport::should_post_vthread_mount()`. > Also, it'd be nice to add a comment explaining why the event posting is > postponed to the `unmount` end point. Fixed and added comment. > 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_unmou... Yes, I see your idea to get rid of the pending unmount event code. Before commenting on that, note that we still need to check if the freeze failed to undo the transition, which would call for this RAII object that we currently have. So in line with your suggestion we could call `VTMS_vthread_mount()` in `~JvmtiUnmountBeginMark()` which would also do the right thing too. Something like this: https://github.com/pchilano/jdk/commit/1729b98f554469fedbbce52333eccea9d1c81514 We can go this simplified route, but note that we would post unmount/mount events even if we never unmounted or remounted because freeze failed. It's true that that is how it currently works when unmounting from Java fails, so I guess it's not new behavior. Maybe we could go with this simplified code now and work on it later. I think the unmount event should be always posted at the end of the transition, in `JvmtiVTMSTransitionDisabler::VTMS_unmount_end()`. I know that at that point we have already switched identity to the carrier, but does the specs say the event has to be posted in the context of the vthread? If we can do that then we could keep the simplified version and avoid this extra unmount/mount events. > Is this posted after the VirtualThreadMount extension event posted? > It's posted before. We post the mount event at the end of thaw only if we are able to acquire the monitor: https://github.com/openjdk/jdk/blob/124efa0a6b8d05909e10005f47f06357b2a73949/src/hotspot/share/runtime/continuationFreezeThaw.cpp#L1620 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823319745 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823322449 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823324965 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823323891 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830220838 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830225909
