RFR: 8343132: Remove temporary transitions from Virtual thread implementation

[David Holmes]

On Mon, 28 Oct 2024 08:34:14 GMT, Alan Bateman <alanb at openjdk.org> wrote:

> This is an update to the Virtual thread implementation that we'd like to integrate in advance of JEP 491.
> 
> The update removes the use of  "temporary transitions", basically cases where the thread identity switches to the carrier thread to do something in the context of the carrier while a virtual thread is mounted. These cases create complexity for JVMTI and observability tools. It has also attracted attention in the review of the JEP 491 implementation as the object monitor changes have to deal with the possibility of entering monitors while in this state. There are 3 usages changes:
> 
> 1. In submitRunContinuation the submit to the scheduler is changed so that it executes in the context of a virtual thread for cases where one virtual thread unparks another. This requires pinning to prevent preemption during this sensitive operation. ForkJoinPool.poolSubmit is changed so that it uses the identity of the carrier. This change has no impact on the uses of lazySubmit or externalSubmit.
> 2. Timed-park. The current implementation schedules/cancels the timer task with the virtual thread mounted. This runs in the context of the carrier as any contention would infer with thread state, park blocker and the parking permit. The implementation is changed to schedule the timeout after unmounting, and to cancel before re-mounting. The downside of this is that it will scheduled later (maybe 200us later than before). We could capture the time and adjust but it doesn't seem worth it.
> 3. jdk.tracePinnedThreads. This is a diagnostic option for finding usages of thread locals in code executed by virtual threads. This is changed so use a thread local to detect reentrancy.
> 
> The changes means that notifyJvmtiHideFrames, its intrinsic, and the JVMTI "tmp VTMS_transition" bit go away.

Hotspot cleanup looks great! It is really good to see this temporary transition logic go away.

src/java.base/share/classes/java/lang/ThreadLocal.java line 813:

> 811: 
> 812:     /**
> 813:      * Print the print stack of the current thread, skipping the printStackTrace frame.

Suggestion:

     * Print the stack of the current thread, skipping the printStackTrace frame.

src/java.base/share/classes/java/lang/VirtualThread.java line 537:

> 535:                 assert parkTimeout > 0;
> 536:                 timeoutTask = schedule(this::unpark, parkTimeout, NANOSECONDS);
> 537:                 setState(newState = TIMED_PARKED);

Just to be clear here, if the timeout expires before we can call `setState`, the unpark is basically a no-op, and we will see that we have been unparked at line 541 and set the state correctly to UNPARKED.

test/jdk/java/lang/Thread/virtual/ParkWithFixedThreadPool.java line 93:

> 91:             } finally {
> 92:                 // ExecutorService::execute may consume parking permit
> 93:                 LockSupport.unpark(Thread.currentThread());

This seems a bit odd - why would the current thread need to unpark itself? Why should it have a park permit available here?

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21735#pullrequestreview-2406915529
PR Review Comment: https://git.openjdk.org/jdk/pull/21735#discussion_r1823761637
PR Review Comment: https://git.openjdk.org/jdk/pull/21735#discussion_r1823766067
PR Review Comment: https://git.openjdk.org/jdk/pull/21735#discussion_r1823767061