On Mon, 28 Oct 2024 08:34:14 GMT, Alan Bateman <[email protected]> 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
