On Mon, 21 Oct 2024 15:41:45 GMT, Axel Boldt-Christmas <abold...@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/runtime/javaThread.cpp line 1545: > >> 1543: if (is_vthread_mounted()) { >> 1544: // _lock_id is the thread ID of the mounted virtual thread >> 1545: st->print_cr(" Carrying virtual thread #" INT64_FORMAT, >> lock_id()); > > What is the interaction here with `switchToCarrierThread` and the window > between? > > carrier.setCurrentThread(carrier); > Thread.setCurrentLockId(this.threadId()); > > Will we print the carrier threads id as a virtual threads id? (I am guessing > that is_vthread_mounted is true when switchToCarrierThread is called). Just to say that we hope to eventually remove these "temporary transitions". This PR brings in a change that we've had in the loom repo to not need this when calling out to the scheduler. The only significant remaining use is timed-park. Once we address that then we will remove the need to switch the thread identity and remove some complexity, esp. for JVMTI and serviceability. In the mean-time, yes, the JavaThread.lock_id will temporarily switch to the carrier so a thread-dump/safepoint at just the right time looks like it print will be tid of the carrier rather than the mounted virtual thread. So we should fix that. (The original code in main line skipped this case so was lossy when taking a thread dump when hitting this case, David might remember the discussion on that issue). > src/java.base/share/classes/jdk/internal/vm/Continuation.java line 62: > >> 60: NATIVE(2, "Native frame or <clinit> on stack"), >> 61: MONITOR(3, "Monitor held"), >> 62: CRITICAL_SECTION(4, "In critical section"); > > Is there a reason that the `reasonCode` values does not match the > `freeze_result` reason values used in `pinnedReason(int reason)` to create > one of these? > > I cannot see that it is used either. Only seem to be read for JFR > VirtualThreadPinned Event which only uses the string. That's a good question as they should match. Not noticed as it's not currently used. As it happens, this has been reverted in the loom repo as part of improving this code and fixing another issue. Related is the freeze_result enum has new members, e.g. freeze_unsupported for LM_LEGACY, that don't have a mapping to a Pinned, need to check if we could trip over that. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810578179 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827316145
