RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning

Wed Nov 6 17:39:53 UTC 2024

On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo <pchilanomate at 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