On Thu, 17 Oct 2024 14:28:30 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 threads don't need to worry about holding monitors anymo... Been learning a ton by reading the code changes and questions/answers from/to others. But I still have some questions (and some small suggestions). I'm done reviewing this piece of good-looking code, and I really enjoyed it. Thanks! src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 945: > 943: > 944: void inc_held_monitor_count(); > 945: void dec_held_monitor_count(); I prefer to pass the `tmp` register as it's done in PPC. Manual register allocation is hard as it is, hiding what registers are clobbered makes it even harder. Suggestion: void inc_held_monitor_count(Register tmp); void dec_held_monitor_count(Register tmp); src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 740: > 738: void MacroAssembler::clobber_nonvolatile_registers() { > 739: BLOCK_COMMENT("clobber nonvolatile registers {"); > 740: Register regs[] = { Maybe I've worked in the embedded world for too, but it's always faster and safer to store arrays with values that never change in read only memory. Suggestion: static const Register regs[] = { src/hotspot/cpu/riscv/continuationFreezeThaw_riscv.inline.hpp line 273: > 271: ? frame_sp + fsize - frame::sender_sp_offset > 272: // we need to re-read fp because it may be an oop and we might > have fixed the frame. > 273: : *(intptr_t**)(hf.sp() - 2); Suggestion: : *(intptr_t**)(hf.sp() - frame::sender_sp_offset); src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 793: > 791: > 792: void inc_held_monitor_count(Register tmp = t0); > 793: void dec_held_monitor_count(Register tmp = t0); I prefer if we don't use any default argument. Manual register allocation is hard as it is, hiding what registers are clobbered makes it even harder. Also it would make it more in line with how it's done in PPC. Suggestion: void inc_held_monitor_count(Register tmp); void dec_held_monitor_count(Register tmp); src/hotspot/share/runtime/continuation.cpp line 125: > 123: }; > 124: > 125: static bool is_safe_vthread_to_preempt_for_jvmti(JavaThread* target, oop > vthread) { I think the code reads better if you change to `is_safe_to_preempt_vthread_for_jvmti`. Suggestion: static bool is_safe_to_preempt_vthread_for_jvmti(JavaThread* target, oop vthread) { src/hotspot/share/runtime/continuation.cpp line 135: > 133: #endif // INCLUDE_JVMTI > 134: > 135: static bool is_safe_vthread_to_preempt(JavaThread* target, oop vthread) { I think the code reads better if you change to `is_safe_to_preempt_vthread`. Suggestion: static bool is_safe_to_preempt_vthread(JavaThread* target, oop vthread) { src/hotspot/share/runtime/continuation.hpp line 66: > 64: > 65: enum preempt_kind { > 66: freeze_on_monitorenter = 1, Is there a reason why the first enumerator doesn't start at zero? src/hotspot/share/runtime/continuationFreezeThaw.cpp line 889: > 887: return f.is_native_frame() ? recurse_freeze_native_frame(f, caller) > : recurse_freeze_stub_frame(f, caller); > 888: } else { > 889: return freeze_pinned_native; Can you add a comment about why you only end up here for `freeze_pinned_native`, cause that is not clear to me. src/hotspot/share/runtime/objectMonitor.cpp line 1193: > 1191: } > 1192: > 1193: assert(node->TState == ObjectWaiter::TS_ENTER || node->TState == > ObjectWaiter::TS_CXQ, ""); In `ObjectMonitor::resume_operation()` the exact same line is a `guarantee`- not an `assert`-line, is there any reason why? ------------- PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2404133418 Marked as reviewed by fbredberg (Committer). PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2410872086 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822551094 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822696920 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822200193 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1822537887 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824253403 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824255622 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824262945 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824405820 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1824676122
