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

[Patricio Chilano Mateo]

On Tue, 22 Oct 2024 13:51:26 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/share/runtime/continuationFreezeThaw.cpp line 2234:
> 
>> 2232:     retry_fast_path = true;
>> 2233:   } else {
>> 2234:     relativize_chunk_concurrently(chunk);
> 
> Is the `relativize_chunk_concurrently` solution to the race only to have a single flag read in `can_thaw_fast` or is there some other subtlety here? 
> 
> While not required for the PR, if it is just to optimise the `can_thaw_fast` check, it can probably be made to work with one load and still allow concurrent gcs do fast_thaw when we only get here due to a lockstack.

Yes, it's just to do a single read. I guess you are thinking of combining flags and lockStackSize into a int16_t?

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 2247:
> 
>> 2245:     _thread->lock_stack().move_from_address(tmp_lockstack, lockStackSize);
>> 2246: 
>> 2247:     chunk->set_lockstack_size(0);
> 
> After some discussion here at the office we think there might be an issue here with simply hiding the oops without clearing them. Below in `recurse_thaw` we `do_barriers`. But it does not touch these lockstack. Missing the SATB store barrier is probably fine from a liveness perspective, because the oops in the lockstack must also be in the frames. But removing the oops without a barrier and clear will probably lead to problems down the line.
> 
> Something like the following would probably handle this. Or even fuse the `copy_lockstack` and `clear_lockstack` together into some kind of `transfer_lockstack` which both loads and clears the oops.
> 
> 
> diff --git a/src/hotspot/share/oops/stackChunkOop.cpp b/src/hotspot/share/oops/stackChunkOop.cpp
> index d3d63533eed..f737bd2db71 100644
> --- a/src/hotspot/share/oops/stackChunkOop.cpp
> +++ b/src/hotspot/share/oops/stackChunkOop.cpp
> @@ -470,6 +470,28 @@ void stackChunkOopDesc::copy_lockstack(oop* dst) {
>    }
>  }
>  
> +void stackChunkOopDesc::clear_lockstack() {
> +  const int cnt = lockstack_size();
> +  const bool requires_gc_barriers = is_gc_mode() || requires_barriers();
> +  const bool requires_uncompress = has_bitmap() && UseCompressedOops;
> +  const auto clear_obj = [&](intptr_t* at) {
> +    if (requires_uncompress) {
> +      HeapAccess<>::oop_store(reinterpret_cast<narrowOop*>(at), nullptr);
> +    } else {
> +      HeapAccess<>::oop_store(reinterpret_cast<oop*>(at), nullptr);
> +    }
> +  };
> +
> +  if (requires_gc_barriers) {
> +    intptr_t* lockstack_start = start_address();
> +    for (int i = 0; i < cnt; i++) {
> +      clear_obj(&lockstack_start[i]);
> +    }
> +  }
> +  set_lockstack_size(0);
> +  set_has_lockstack(false);
> +}
> +
>  void stackChunkOopDesc::print_on(bool verbose, outputStream* st) const {
>    if (*((juint*)this) == badHeapWordVal) {
>      st->print_cr("BAD WORD");
> diff --git a/src/hotspot/share/oops/stackChunkOop.hpp b/src/hotspot/share/oops/stackChunkOop.hpp
> index 28e0576801e..928e94dd695 100644
> --- a/src/hotspot/share/oops/stackChunkOop.hpp
> +++ b/src/hotspot/share/oops/stackChunkOop.hpp
> @@ -167,6 +167,7 @@ class stackChunkOopDesc : public instanceOopDesc {
>    void fix_thawed_frame(const frame& f, const RegisterMapT* map);
>  
>    void copy_lockstack(oop* start);
> +  void clear_lockstack();
>  
>    template <typename OopT, class StackChunkLockSta...

Ok, I'll change copy_lockstack to both load and clear the oops in the same method. Now, when we call do_barriers on recurse_thaw we don't clear the oops, we just load and store the loaded value again. Is it the case that we just need to do a store, so that already works, or are we missing clearing the oops from the copied frames?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811244844
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811244206