8343132: Remove temporary transitions from Virtual thread implementation #21735

AlanBateman · 2024-10-28T08:34:14Z

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:

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8343132: Remove temporary transitions from Virtual thread implementation (Enhancement - P3)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) Review applies to c88ce3dd
Patricio Chilano Mateo (@pchilano - Reviewer) Review applies to c88ce3dd
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21735/head:pull/21735
$ git checkout pull/21735

Update a local copy of the PR:
$ git checkout pull/21735
$ git pull https://git.openjdk.org/jdk.git pull/21735/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21735

View PR using the GUI difftool:
$ git pr show -t 21735

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21735.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-28T08:34:48Z

👋 Welcome back alanb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-28T08:34:50Z

@AlanBateman This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343132: Remove temporary transitions from Virtual thread implementation

Reviewed-by: dholmes, sspitsyn, pchilanomate

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-28T08:35:27Z

@AlanBateman The following labels will be automatically applied to this pull request:

core-libs
graal
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-29T06:26:35Z

Webrevs

01: Full - Incremental (2d18c116)
00: Full (c88ce3dd)

sspitsyn

The fix looks good to me. It is important and nice simplification.
The JVMTI part is strait forward.

pchilano

Looks good to me. Thanks for removing this.

dholmes-ora

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

dholmes-ora · 2024-10-31T04:33:47Z

src/java.base/share/classes/java/lang/ThreadLocal.java

@@ -811,30 +810,28 @@ private static boolean traceVirtualThreadLocals() {
    }

    /**
-     * Print a stack trace if the current thread is a virtual thread.
+     * Print the print stack of the current thread, skipping the printStackTrace frame.


Suggested change

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

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

dholmes-ora · 2024-10-31T04:43:21Z

src/java.base/share/classes/java/lang/VirtualThread.java

+                timeoutTask = schedule(this::unpark, parkTimeout, NANOSECONDS);
+                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.

Yes, and same thing if unparked by some other thread while the target thread is parking. We have several tests that bash on this.

dholmes-ora · 2024-10-31T04:45:35Z

test/jdk/java/lang/Thread/virtual/ParkWithFixedThreadPool.java

+                // ExecutorService::execute may consume parking permit
+                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?

In this test, Scheduler.execute method will consume the current thread's parking permit when there is contention on the queue. In a well behaved system, all usages of park will first test some condition before parking. This test doesn't do this, hence it created the scenario where parking after unparking might hang. Previous discussion in loom/pull/59. There is no support exposed for doing custom schedulers at this time but this is the type of thing that comes up so we kept the test.

AlanBateman · 2024-10-31T08:52:09Z

/integrate

openjdk · 2024-10-31T08:53:20Z

Going to push as commit dee0982.
Since your change was applied there have been 2 commits pushed to the master branch:

2f1ba5e: 8343307: Throw ZipException instead of IAE in ZipFile.Source::initCEN
388d44f: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-31T08:53:27Z

@AlanBateman Pushed as commit dee0982.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Initial commit

c88ce3d

openjdk bot added graal [email protected] serviceability [email protected] hotspot [email protected] core-libs [email protected] labels Oct 28, 2024

AlanBateman marked this pull request as ready for review October 29, 2024 06:22

openjdk bot added the rfr Pull request is ready for review label Oct 29, 2024

graalvmbot mentioned this pull request Oct 29, 2024

[JDK-8343193] Adapt JDK-8343132: Remove temporary transitions from Virtual thread implementation oracle/graal#9980

Merged

sspitsyn approved these changes Oct 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2024

pchilano approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'master' into JDK-8343132

90def08

dholmes-ora approved these changes Oct 31, 2024

View reviewed changes

AlanBateman added 2 commits October 31, 2024 06:56

Merge branch 'master' into JDK-8343132

dc46d4a

Fix typo in comment

2d18c11

openjdk bot removed the ready Pull request is ready to be integrated label Oct 31, 2024

dholmes-ora approved these changes Oct 31, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 31, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 31, 2024

openjdk bot closed this Oct 31, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343132: Remove temporary transitions from Virtual thread implementation #21735

8343132: Remove temporary transitions from Virtual thread implementation #21735

AlanBateman commented Oct 28, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 28, 2024

openjdk bot commented Oct 28, 2024 •

edited

Loading

openjdk bot commented Oct 28, 2024

mlbridge bot commented Oct 29, 2024 •

edited

Loading

sspitsyn left a comment

pchilano left a comment

dholmes-ora left a comment

dholmes-ora Oct 31, 2024

dholmes-ora Oct 31, 2024

AlanBateman Oct 31, 2024 •

edited

Loading

dholmes-ora Oct 31, 2024

AlanBateman Oct 31, 2024

AlanBateman commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

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

		timeoutTask = schedule(this::unpark, parkTimeout, NANOSECONDS);
		setState(newState = TIMED_PARKED);

		// ExecutorService::execute may consume parking permit
		LockSupport.unpark(Thread.currentThread());

8343132: Remove temporary transitions from Virtual thread implementation #21735

8343132: Remove temporary transitions from Virtual thread implementation #21735

Conversation

AlanBateman commented Oct 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 28, 2024

openjdk bot commented Oct 28, 2024 • edited Loading

openjdk bot commented Oct 28, 2024

mlbridge bot commented Oct 29, 2024 • edited Loading

Webrevs

sspitsyn left a comment

Choose a reason for hiding this comment

pchilano left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Oct 31, 2024

Choose a reason for hiding this comment

dholmes-ora Oct 31, 2024

Choose a reason for hiding this comment

AlanBateman Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

dholmes-ora Oct 31, 2024

Choose a reason for hiding this comment

AlanBateman Oct 31, 2024

Choose a reason for hiding this comment

AlanBateman commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

AlanBateman commented Oct 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 28, 2024 •

edited

Loading

mlbridge bot commented Oct 29, 2024 •

edited

Loading

AlanBateman Oct 31, 2024 •

edited

Loading