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

[Patricio Chilano Mateo]

On Tue, 29 Oct 2024 21:32:44 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/share/oops/method.cpp line 870:
> 
>> 868: }
>> 869: 
>> 870: bool Method::is_object_wait0() const {
> 
> It might be worth mentioning that is not a general-purpose API, so we don't have to worry about false positives here.

Right, I added a check for the klass too.

> src/hotspot/share/oops/stackChunkOop.inline.hpp line 255:
> 
>> 253:                          RegisterMap::WalkContinuation::include);
>> 254:     full_map.set_include_argument_oops(false);
>> 255:     closure->do_frame(f, map);
> 
> This could use a comment.  I guess we weren't looking at the stub frame before, only the caller.  Why is this using `map` instead of `full_map`?

The full map gets only populated once we get the sender. We only need it when processing the caller which needs to know where each register was spilled since it might contain an oop.

> src/hotspot/share/prims/jvmtiEnv.cpp line 1363:
> 
>> 1361:   }
>> 1362: 
>> 1363:   if (LockingMode == LM_LEGACY && java_thread == nullptr) {
> 
> Do we need to check for `java_thread == nullptr` for other locking modes?

No, both LM_LIGHTWEIGHT and LM_MONITOR have support for virtual threads. LM_LEGACY doesn't, so if the virtual thread is unmounted we know there is no monitor information to collect.

> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1602:
> 
>> 1600:           // If the thread was found on the ObjectWaiter list, then
>> 1601:           // it has not been notified.
>> 1602:           Handle th(current_thread, w->threadObj());
> 
> Why use get_vthread_or_thread_oop() above but threadObj()?  It probably needs a comment.

We already filtered virtual threads above so no point in calling get_vthread_or_thread_oop() again. They will actually return the same result though.

> src/hotspot/share/runtime/continuation.hpp line 50:
> 
>> 48: class JavaThread;
>> 49: 
>> 50: // should match Continuation.toPreemptStatus() in Continuation.java
> 
> I can't find Continuation.toPreemptStatus() and the enum in Continuation.java doesn't match.

Should be just PreemptStatus. Fixed.

> src/hotspot/share/runtime/continuationEntry.cpp line 51:
> 
>> 49:   _return_pc = nm->code_begin() + _return_pc_offset;
>> 50:   _thaw_call_pc = nm->code_begin() + _thaw_call_pc_offset;
>> 51:   _cleanup_pc = nm->code_begin() + _cleanup_offset;
> 
> I don't see why we need these relative offsets.  Instead of doing
> 
> _thaw_call_pc_offset = __ pc() - start;
> 
> why not do
> 
> _thaw_call_pc = __ pc();
> 
> The only reason for the offsets would be if what gen_continuation_enter() generated was going to be relocated, but I don't think it is.

But these are generated in a temporary buffer. Until we call nmethod::new_native_nmethod() we won't know the final addresses.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821695166
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821695964
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821697629
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821698318
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821698705
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821699155