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

[Patricio Chilano Mateo]

On Tue, 29 Oct 2024 01:59:35 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix comment in VThreadWaitReenter
>
> src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp line 1351:
> 
>> 1349:   // set result handler
>> 1350:   __ mov(result_handler, r0);
>> 1351:   __ str(r0, Address(rfp, frame::interpreter_frame_result_handler_offset * wordSize));
> 
> I'm guessing this is here because preemption doesn't save/restore registers, even callee-saved registers, so we need to save this somewhere.  I think this deserves a comment.

Added comment.

> src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp line 1509:
> 
>> 1507:     Label no_oop;
>> 1508:     __ adr(t, ExternalAddress(AbstractInterpreter::result_handler(T_OBJECT)));
>> 1509:     __ ldr(result_handler, Address(rfp, frame::interpreter_frame_result_handler_offset*wordSize));
> 
> We only need this when preempted, right?  So could this be moved into the block above, where we call restore_after_resume()?

Moved.

> src/hotspot/cpu/x86/c1_Runtime1_x86.cpp line 643:
> 
>> 641: uint Runtime1::runtime_blob_current_thread_offset(frame f) {
>> 642: #ifdef _LP64
>> 643:   return r15_off / 2;
> 
> I think using r15_offset_in_bytes() would be less confusing.

I copied the same comments the other platforms have to make it more clear.

> src/hotspot/cpu/x86/interp_masm_x86.cpp line 359:
> 
>> 357:   push_cont_fastpath();
>> 358: 
>> 359:   // Make VM call. In case of preemption set last_pc to the one we want to resume to.
> 
> From the comment, it sounds like we want to set last_pc to resume_pc, but I don't see that happening.  The push/pop of rscratch1 doesn't seem to be doing anything.

Method `MacroAssembler::call_VM_helper()` expects the current value at the top of the stack to be the last_java_pc. There is comment on that method explaining it:  https://github.com/openjdk/jdk/blob/60364ef0010bde2933c22bf581ff8b3700c4afd6/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L1658

> src/hotspot/share/c1/c1_Runtime1.hpp line 138:
> 
>> 136:   static void initialize_pd();
>> 137: 
>> 138:   static uint runtime_blob_current_thread_offset(frame f);
> 
> I think this returns an offset in wordSize units, but it's not documented.  In some places we always return an offset in bytes and let the caller convert.

Added comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821591515
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821593810
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821592920
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821593351
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821591930