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

[Patricio Chilano Mateo]

On Tue, 29 Oct 2024 23:05:20 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/runtime/continuationFreezeThaw.cpp line 696:
> 
>> 694:   // in a fresh chunk, we freeze *with* the bottom-most frame's stack arguments.
>> 695:   // They'll then be stored twice: in the chunk and in the parent chunk's top frame
>> 696:   const int chunk_start_sp = cont_size() + frame::metadata_words + _monitors_in_lockstack;
> 
> `cont_size() + frame::metadata_words + _monitors_in_lockstack` is used more than once.  Would it make sense to add a helper function named something like `total_cont_size()`?

Maybe, but I only see it twice, not sure we gain much. Also we save having to jump back and forth to see what total_cont_size() would actually account for.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1063:
> 
>> 1061:   unwind_frames();
>> 1062: 
>> 1063:   chunk->set_max_thawing_size(chunk->max_thawing_size() + _freeze_size - _monitors_in_lockstack - frame::metadata_words);
> 
> It seems a little weird to subtract these here only to add them back in other places (see my comment above suggesting total_cont_size).  I wonder if there is a way to simply these adjustments.  Having to replicate _monitors_in_lockstack +- frame::metadata_words in lots of places seems error-prone.

The reason why this is added and later subtracted is because when allocating the stackChunk we need to account for all space needed, but when specifying how much space the vthread needs in the stack to allocate the frames we don't need to count _monitors_in_lockstack. I'd rather not group it with frame::metadata_words because these are logically different things. In fact, if we never subtract frame::metadata_words when setting max_thawing_size we should not need to account for it in thaw_size() (this is probably something we should clean up in the future). But for _monitors_in_lockstack we always need to subtract it to max_thawing_size.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1842:
> 
>> 1840:   size += frame::metadata_words; // For the top pc+fp in push_return_frame or top = stack_sp - frame::metadata_words in thaw_fast
>> 1841:   size += 2*frame::align_wiggle; // in case of alignments at the top and bottom
>> 1842:   size += frame::metadata_words; // for preemption case (see possibly_adjust_frame)
> 
> So this means it's OK to over-estimate the size here?

Yes, this will be the space allocated in the stack by the vthread when thawing.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2062:
> 
>> 2060:   }
>> 2061: 
>> 2062:   f.next(SmallRegisterMap::instance, true /* stop */);
> 
> Suggestion:
> 
>   f.next(SmallRegisterMap::instance(), true /* stop */);
> 
> This looks like a typo, so I wonder how it compiled.  I guess template magic is hiding it.

Fixed.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2650:
> 
>> 2648:     _cont.tail()->do_barriers<stackChunkOopDesc::BarrierType::Store>(_stream, &map);
>> 2649:   } else {
>> 2650:     _stream.next(SmallRegisterMap::instance);
> 
> Suggestion:
> 
>     _stream.next(SmallRegisterMap::instance());

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823486049
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823487296
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823488795
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823502075
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1823503636