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

[Patricio Chilano Mateo]

On Wed, 23 Oct 2024 22:59:19 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Add comments for Coleen
>>  - Fix JvmtiUnmountBeginMark
>
> src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 135:
> 
>> 133:   assert(*f.addr_at(frame::interpreter_frame_last_sp_offset) == 0, "should be null for top frame");
>> 134:   intptr_t* lspp = f.addr_at(frame::interpreter_frame_last_sp_offset);
>> 135:   *lspp = f.unextended_sp() - f.fp();
> 
> Can you write a comment what this is doing briefly and why?

Added comment.

> src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 1550:
> 
>> 1548: #endif /* ASSERT */
>> 1549: 
>> 1550:   push_cont_fastpath();
> 
> One of the callers of this gives a clue what it does.
> 
>   __ push_cont_fastpath(); // Set JavaThread::_cont_fastpath to the sp of the oldest interpreted frame we know about
> 
> Why do you do this here?  Oh please more comments...

_cont_fastpath is what we check in freeze_internal to decide if we can take the fast path. Since we are calling from the interpreter we have to take the slow path. Added a comment.

> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 2032:
> 
>> 2030:     // Force freeze slow path in case we try to preempt. We will pin the
>> 2031:     // vthread to the carrier (see FreezeBase::recurse_freeze_native_frame()).
>> 2032:     __ push_cont_fastpath();
> 
> We need to do this because we might freeze, so JavaThread::_cont_fastpath should be set in case we do?

Right. We want to take the slow path to find the compiled native wrapper frame and fail to freeze. Otherwise the fast path won't find it since we don't walk the stack.

> src/hotspot/share/runtime/continuation.cpp line 89:
> 
>> 87:       // we would incorrectly throw it during the unmount logic in the carrier.
>> 88:       if (_target->has_async_exception_condition()) {
>> 89:         _failed = false;
> 
> This says "Don't" but then failed is false which doesn't make sense.  Should it be true?

Yes, good catch.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1275:
> 
>> 1273: 
>> 1274:   if (caller.is_interpreted_frame()) {
>> 1275:     _total_align_size += frame::align_wiggle;
> 
> Please put a comment here about frame align-wiggle.

I removed this case since it can never happen. The caller has to be compiled, and we assert that at the beginning. This was a leftover from the forceful preemption at a safepoint work. I removed the similar code in recurse_thaw_stub_frame. I added a comment for the compiled and native cases though.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1278:
> 
>> 1276:   }
>> 1277: 
>> 1278:   patch(f, hf, caller, false /*is_bottom_frame*/);
> 
> I also forgot what patch does.  Can you add a comment here too?

I added a comment where it is defined since it is used in several places.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1552:
> 
>> 1550:   assert(!cont.is_empty(), "");
>> 1551:   // This is done for the sake of the enterSpecial frame
>> 1552:   StackWatermarkSet::after_unwind(thread);
> 
> Is there a new place for this StackWatermark code?

I removed it. We have already processed the enterSpecial frame as part of flush_stack_processing(), in fact we processed up to the caller of `Continuation.run()`.

> src/hotspot/share/runtime/objectMonitor.cpp line 876:
> 
>> 874:   // and in doing so avoid some transitions ...
>> 875: 
>> 876:   // For virtual threads that are pinned do a timed-park instead, to
> 
> I had trouble parsing this first sentence.  I think it needs a comma after pinned and remove the comma after instead.

Fixed.

> src/hotspot/share/runtime/objectMonitor.cpp line 2305:
> 
>> 2303: }
>> 2304: 
>> 2305: void ObjectMonitor::Initialize2() {
> 
> Can you put a comment why there's a second initialize function?  Presumably after some state is set.

Added comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816658344
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816660065
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816660542
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816660817
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816661388
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816661733
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816662247
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816662554
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816663065