On Tue, 29 Oct 2024 02:09:24 GMT, Dean Long <dl...@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/cpu/x86/c1_Runtime1_x86.cpp line 223: > >> 221: } >> 222: >> 223: void StubAssembler::epilogue(bool use_pop) { > > Is there a better name we could use, like `trust_fp` or `after_resume`? I think `trust_fp` would be confusing because at this point rfp will have an invalid value and we don't want to use it to restore sp, i.e. we should not trust fp. And `after_resume` wouldn't always apply since we don't always preempt. The `use_pop` name was copied form x64, but I think it's still fine here. We also have the comment right below this line which explains why we don't want to use `leave()` and instead pop the top words from the stack. > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 324: > >> 322: movq(scrReg, tmpReg); >> 323: xorq(tmpReg, tmpReg); >> 324: movptr(boxReg, Address(r15_thread, JavaThread::lock_id_offset())); > > I don't know if it helps to schedule this load earlier (it is used in the > next instruction), but it probably won't hurt. I moved it before `movq(scrReg, tmpReg)` since we need `boxReg` above, but I don't think this will change anything. > src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1659: > >> 1657: int i = 0; >> 1658: for (frame f = freeze_start_frame(); >> Continuation::is_frame_in_continuation(ce, f); f = f.sender(&map), i++) { >> 1659: if (!((f.is_compiled_frame() && !f.is_deoptimized_frame()) || (i >> == 0 && (f.is_runtime_frame() || f.is_native_frame())))) { > > OK, `i == 0` just means first frame here, so you could use a bool instead of > an int, or even check for f == freeze_start_frame(), right? Changed to use boolean `is_top_frame`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1831594384 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1831597325 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1831599268
