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

[Dean Long]

On Mon, 28 Oct 2024 18:58:29 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> regardless of when you freeze, while doing the freezing the monitor could have been released already. So trying to acquire the monitor after freezing can always succeed, which means we don't want to unmount but continue execution, i.e cancel the preemption.

Is this purely a performance optimization, or is there a correctness issue if we don't notice the monitor was released and cancel the preemption?  It seems like the monitor can be released at any time, so what makes freeze special that we need to check afterwards?  We aren't doing the monitor check atomically, so the monitor could get released right after we check it.  So I'm guessing we choose to check after freeze because freeze has non-trivial overhead.

>> src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 159:
>> 
>>> 157: 
>>> 158:   // The interpreter native wrapper code adds space in the stack equal to size_of_parameters()
>>> 159:   // after the fixed part of the frame. For wait0 this is equal to 3 words (this + long parameter).
>> 
>> Suggestion:
>> 
>>   // after the fixed part of the frame. For wait0 this is equal to 2 words (this + long parameter).
>> 
>> Isn't that 2 words, not 3?
>
> The timeout parameter is a long which we count as 2 words: https://github.com/openjdk/jdk/blob/0e3fc93dfb14378a848571a6b83282c0c73e690f/src/hotspot/share/runtime/signature.hpp#L347
> I don't know why we do that for 64 bits.

OK, I think there are historical or technical reasons why it's hard to change, because of the way the JVM spec is written.

>> src/hotspot/cpu/aarch64/frame_aarch64.hpp line 77:
>> 
>>> 75:     // Interpreter frames
>>> 76:     interpreter_frame_result_handler_offset          =  3, // for native calls only
>>> 77:     interpreter_frame_oop_temp_offset                =  2, // for native calls only
>> 
>> This conflicts with sender_sp_offset.  Doesn't that cause a problem?
>
> No, it just happens to be stored at the sender_sp marker. We were already making room for two words but only using one.

`sender_sp_offset` is listed under "All frames", but I guess that's wrong and should be changed.  Can we fix the comments to match x86, which lists this offset under "non-interpreter frames"?

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

OK, I was looking for where it stores it in the anchor, but it doesn't, at least not until make_walkable() is called.

>> src/hotspot/share/code/nmethod.cpp line 1302:
>> 
>>> 1300:     _compiler_type           = type;
>>> 1301:     _orig_pc_offset          = 0;
>>> 1302:     _num_stack_arg_slots     = 0;
>> 
>> Was the old value wrong, unneeded, or is this set somewhere else?  If this field is not used, then we might want to set it to an illegal value in debug builds.
>
> We read this value from the freeze/thaw code in several places. Since the only compiled native frame we allow to freeze is Object.wait0 the old value would be zero too. But I think the correct thing is to just set it to zero always since a value > 0 is only meaningful for Java methods.

Isn't it possible that we might allow more compiled native frames in the future, and then we would have to undo this change?  I think this change should be reverted.  If continuations code wants to assert that this is 0, then that should be in continuations code, the nmethod code doesn't need to know how this field is used.  However, it looks like continuations code is the only client of this field, so I can see how it would be tempting to just set it to 0 here, but it doesn't feel right.

>> src/hotspot/share/runtime/continuation.hpp line 50:
>> 
>>> 48: class JavaThread;
>>> 49: 
>>> 50: // should match Continuation.PreemptStatus() in Continuation.java
>> 
>> As far as I can tell, these enum values still don't match the Java values.  If they need to match, then maybe there should be asserts that check that.
>
> `PreemptStatus` is meant to be used with `tryPreempt()` which is not implemented yet, i.e. there is no method yet that maps between these values and the PreemptStatus enum. The closest is `Continuation.pinnedReason` which we do use. So if you want I can remove the reference to PreemptStatus and use pinnedReason instead.

Yes, that would be better for now.

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

PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2442880740
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1819705281
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821524020
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821705135
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823572138
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823584967