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

Tue Oct 22 02:23:16 UTC 2024

On Mon, 21 Oct 2024 07:57:31 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/continuationFreezeThaw_aarch64.inline.hpp line 300:
> 
>> 298:   CodeBlob* cb = top.cb();
>> 299: 
>> 300:   if (cb->frame_size() == 2) {
> 
> Is this a filter to identify c2 runtime stubs? Is there some other property we can check or assert here? This assumes that no other runtime frame will have this size.

We could also check the caller of the runtime frame, something like:

#ifdef ASSERT
  RegisterMap map(JavaThread::current(),
                  RegisterMap::UpdateMap::skip,
                  RegisterMap::ProcessFrames::skip,
                  RegisterMap::WalkContinuation::skip);
  frame caller = top.sender(&map);
  assert(caller.is_compiled_frame(), "");
  assert(cb->frame_size() > 2 || caller.cb()->as_nmethod()->is_compiled_by_c2(), "");
#endif

Ideally we would want to check if cb->frame_size() is different than the actual size of the physical frame.

> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 1275:
> 
>> 1273: void SharedRuntime::continuation_enter_cleanup(MacroAssembler* masm) {
>> 1274:   ::continuation_enter_cleanup(masm);
>> 1275: }
> 
> Now that `continuation_enter_cleanup` is a static member function, just merge the static free function with this static member function.

Since we have 3 free static functions to handle the continuation entry(create, fill, cleanup) I would prefer to keep the cleanup one for consistency. We could also change them all to be members of SharedRuntime. But except for the exception I added for continuation_enter_cleanup(), all these are called by gen_continuation_enter/gen_continuation_yield() which are also static free functions.

> src/hotspot/cpu/x86/assembler_x86.cpp line 2866:
> 
>> 2864:     emit_int32(0);
>> 2865:   }
>> 2866: }
> 
> Is it possible to make this more general and explicit instead of a sequence of bytes?
> 
> Something along the lines of:
> ```C++
>   const address tar = L.is_bound() ? target(L) : pc();
>   const Address adr = Address(checked_cast<int32_t>(tar - pc()), tar, relocInfo::none);
> 
>   InstructionMark im(this);
>   emit_prefix_and_int8(get_prefixq(adr, dst), (unsigned char)0x8D);
>   if (!L.is_bound()) {
>     // Patch @0x8D opcode
>     L.add_patch_at(code(), CodeBuffer::locator(offset() - 1, sect()));
>   }
>   // Register and [rip+disp] operand
>   emit_modrm(0b00, raw_encode(dst), 0b101);
>   // Adjust displacement by sizeof lea instruction
>   int32_t disp = adr.disp() - checked_cast<int32_t>(pc() - inst_mark() + sizeof(int32_t));
>   assert(is_simm32(disp), "must be 32bit offset [rip+offset]");
>   emit_int32(disp);
> 
> 
> and then in `pd_patch_instruction` simply match `op == 0x8D /* lea */`.

I'll test it out but looks fine.

> src/hotspot/share/prims/jvmtiExport.cpp line 1681:
> 
>> 1679:   EVT_TRIG_TRACE(EXT_EVENT_VIRTUAL_THREAD_UNMOUNT, ("[%p] Trg Virtual Thread Unmount event triggered", vthread));
>> 1680: 
>> 1681:   // On preemption JVMTI state rebinding has already happened so get it always direclty from the oop.
> 
> Suggestion:
> 
>   // On preemption JVMTI state rebinding has already happened so get it always directly from the oop.

Fixed.

> src/hotspot/share/runtime/frame.cpp line 535:
> 
>> 533:   assert(get_register_address_in_stub(f, SharedRuntime::thread_register()) == (address)thread_addr, "wrong thread address");
>> 534:   return thread_addr;
>> 535: #endif
> 
> With this ifdef, it seems like this belongs in the platform dependent part of the frame class.

I moved it to the platform dependent files.

> src/hotspot/share/runtime/objectMonitor.hpp line 184:
> 
>> 182:   // - We test for anonymous owner by testing for the lowest bit, therefore
>> 183:   //   DEFLATER_MARKER must *not* have that bit set.
>> 184:   static const int64_t DEFLATER_MARKER = 2;
> 
> The comments here should be updated / removed. They are talking about the lower bits of the owner being unset which is no longer true. (And talks about doing bit tests, which I do not think is done anywhere even without this patch).

Removed the comments.

> src/hotspot/share/runtime/objectMonitor.hpp line 186:
> 
>> 184:   static const int64_t DEFLATER_MARKER = 2;
>> 185: 
>> 186:   int64_t volatile _owner;  // Either tid of owner, ANONYMOUS_OWNER_MARKER or DEFLATER_MARKER.
> 
> Suggestion:
> 
>   int64_t volatile _owner;  // Either tid of owner, NO_OWNER, ANONYMOUS_OWNER or DEFLATER_MARKER.

Fixed.

> src/hotspot/share/runtime/synchronizer.cpp line 1467:
> 
>> 1465:       markWord dmw = inf->header();
>> 1466:       assert(dmw.is_neutral(), "invariant: header=" INTPTR_FORMAT, dmw.value());
>> 1467:       if (inf->is_owner_anonymous() && inflating_thread != nullptr) {
> 
> Are these `LM_LEGACY` + `ANONYMOUS_OWNER` changes still required now that `LM_LEGACY` does no freeze?

Yes, it's just a consequence of using tid as the owner, not really related to freezing. So when a thread inflates a monitor that is already owned we cannot store the BasicLock* in the _owner field anymore, since it can clash with some tid, so we mark it as anonymously owned instead. The owner will fix it here when trying to get the monitor, as we do with LM_LIGHTWEIGHT.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809753868
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809749481
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809749657
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809749805
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809750408
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809750552
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809750685
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809754940