RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v9]

[Patricio Chilano Mateo]

On Wed, 30 Oct 2024 12:48:02 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Rename set/has_owner_anonymous to set/has_anonymous_owner
>>  - Fix comments in javaThread.hpp and Thread.java
>>  - Rename nonce/nounce to seqNo in VirtualThread class
>>  - Remove ObjectMonitor::set_owner_from_BasicLock()
>
> 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