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

Thu Oct 24 19:03:20 UTC 2024

On Thu, 24 Oct 2024 03:38:21 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> This is the implementation of JEP 491: Synchronize Virtual Threads without Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for further details.
>> 
>> In order to make the code review easier the changes have been split into the following initial 4 commits:
>> 
>> - Changes to allow unmounting a virtual thread that is currently holding monitors.
>> - Changes to allow unmounting a virtual thread blocked on synchronized trying to acquire the monitor.
>> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` and its timed-wait variants.
>> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>> 
>> The changes fix pinning issues for all 4 ports that currently implement continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added recently and stand in its own commit after the initial ones.
>> 
>> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default locking mode, (and `LM_MONITOR` which comes for free), but not when using `LM_LEGACY` mode. Note that the `LockingMode` flag has already been deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), with the intention to remove `LM_LEGACY` code in future releases.
>> 
>> 
>> ## Summary of changes
>> 
>> ### Unmount virtual thread while holding monitors
>> 
>> As stated in the JEP, currently when a virtual thread enters a synchronized method or block, the JVM records the virtual thread's carrier platform thread as holding the monitor, not the virtual thread itself. This prevents the virtual thread from being unmounted from its carrier, as ownership information would otherwise go wrong. In order to fix this limitation we will do two things:
>> 
>> - We copy the oops stored in the LockStack of the carrier to the stackChunk when freezing (and clear the LockStack). We copy the oops back to the LockStack of the next carrier when thawing for the first time (and clear them from the stackChunk). Note that we currently assume carriers don't hold monitors while mounting virtual threads.
>> 
>> - For inflated monitors we now record the `java.lang.Thread.tid` of the owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This allows us to tie the owner of the monitor to a `java.lang.Thread` instance, rather than to a JavaThread which is only created per platform thread. The tid is already a 64 bit field so we can ignore issues of the counter wrapping around.
>> 
>> #### General notes about this part:
>> 
>> - Since virtual th...
>
> Patricio Chilano Mateo has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fix comment in objectMonitor.hpp and javaThread.hpp
>  - Skip printing tid when not available

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?

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...

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?

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?

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.

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?

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?

src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1657:

> 1655: }
> 1656: 
> 1657: template<typename ConfigT, bool preempt>

This function is kind of big, do we really want it duplicated to pass preempt as a template parameter?

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.

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813899129
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814081166
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814084085
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814905064
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815015410
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815016232
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815245735
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815036910
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815445109
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815479877