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

Wed Nov 6 17:40:03 UTC 2024

On Wed, 30 Oct 2024 12:48:02 GMT, Fredrik Bredberg <fbredberg 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 th...
>
> 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);

Changed.

> 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[] = {

Added.

> 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);

Changed.

> 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);

Changed.

> 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) {

I renamed it to is_vthread_safe_to_preempt_for_jvmti.

> 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) {

I renamed it to is_vthread_safe_to_preempt, which I think it reads even better.

> 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?

There was one value that meant to be for the regular freeze from java. But it was not used so I removed it.

> 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.

We just found a frame that can't be freezed, most likely the call_stub or upcall_stub which indicate there are further natives frames up the stack. I added a comment.

> 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?

The `guarantee` tries to mimic the one here: https://github.com/openjdk/jdk/blob/ae82cc1ba101f6c566278f79a2e94bd1d1dd9efe/src/hotspot/share/runtime/objectMonitor.cpp#L1613
The assert at the epilogue is probably redundant. Also in `UnlinkAfterAcquire`, the else branch already asserts `ObjectWaiter::TS_CXQ`. I removed it.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825101744
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825108078
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825100526
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825101246
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825107036
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825102359
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825103008
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825104666
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825106368