RFR: 8338383: Implementation of Synchronize Virtual Threads without Pinning [v3]

[Patricio Chilano Mateo]

On Mon, 21 Oct 2024 06:38:28 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with six additional commits since the last revision:
>> 
>>  - Fix comments in objectMonitor.hpp
>>  - Move frame::saved_thread_address() to platform dependent files
>>  - Fix typo in jvmtiExport.cpp
>>  - remove usage of frame::metadata_words in possibly_adjust_frame()
>>  - Fix comments in c2 locking paths
>>  - Revert and simplify changes to c1_Runtime1 on aarch64 and riscv
>
> src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp line 231:
> 
>> 229: 
>> 230: StubFrame::~StubFrame() {
>> 231:   __ epilogue(_use_pop_on_epilogue);
> 
> Can we not hook the `_use_pop_on_epilogue` into `return_state_t`, simplify the constructors and keep the old should_not_reach_here guard for stubs which should not return?
> e.g.
> ```C++
> enum return_state_t {
>   does_not_return, requires_return, requires_pop_epilogue_return
> };
> 
> StubFrame::~StubFrame() {
>   if (_return_state == does_not_return) {
>     __ should_not_reach_here();
>   } else {
>     __ epilogue(_return_state == requires_pop_epilogue_return);
>   }
> }

Yes, that's much better. I changed it in both aarch64 and riscv.

> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 115:
> 
>> 113:   // The object's monitor m is unlocked iff m->owner == nullptr,
>> 114:   // otherwise m->owner may contain a thread id, a stack address for LM_LEGACY,
>> 115:   // or the ANONYMOUS_OWNER constant for LM_LIGHTWEIGHT.
> 
> Comment seems out of place in `LockingMode != LM_LIGHTWEIGHT` code.

I removed this comment about what other values might be stored in _owner since we don't need to handle those cases here.

> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 380:
> 
>> 378:     lea(t2_owner_addr, owner_address);
>> 379: 
>> 380:     // CAS owner (null => current thread id).
> 
> I think we should be more careful when and where we talk about thread id and lock id respectively. Given that `switchToCarrierThread` switches the thread, but not the lock id. We should probably define and talk about the lock id when it comes to locking, as saying thread id may be incorrect. 
> 
> Then there is also the different thread ids, the OS level one, and the java level one. (But not sure how to reconcile this without causing confusion)

Fixed the comments to refer to _lock_id. Even without the switchToCarrierThread case I think that's the correct thing to do.

> src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 313:
> 
>> 311: 
>> 312:     log_develop_trace(continuations, preempt)("adjusted sp for c2 runtime stub, initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT
>> 313:                                               " fp: " INTPTR_FORMAT, p2i(sp + frame::metadata_words), p2i(sp), sp[-2]);
> 
> Is there a reason for the mix of `2` and `frame::metadata_words`?
> 
> Maybe this could be
> ```C++
>     intptr_t* const unadjusted_sp = sp;
>     sp -= frame::metadata_words;
>     sp[-2] = unadjusted_sp[-2];
>     sp[-1] = unadjusted_sp[-1];
> 
>     log_develop_trace(continuations, preempt)("adjusted sp for c2 runtime stub, initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT
>                                               " fp: " INTPTR_FORMAT, p2i(unadjusted_sp), p2i(sp), sp[-2]);

I removed the use of frame::metadata_words from the log statement instead to make it consistent, since we would still implicitly be assuming metadata_words it's 2 words when we do the copying. We could use a memcpy and refer to metadata_words, but I think it is clear this way since we are explicitly talking about the 2 extra words missing from the runtime frame as the comment explains.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809745804
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809746249
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809746397
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809747046