Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support the new changes to the VirtualThread states #18673

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Dec 22, 2023

RUNNABLE=2 state was removed, and all other states are re-numbered
due to the removal of the RUNNABLE state.

Two new states are added: UNPARKED=9 and YIELDED=11.

Currently, VirtualThread.yield incorrectly consumes the parking
permit. The parking permit should only be consumed when continuing
from VirtualThread.park. The two new states are added to address
this incorrect behaviour.

Depends on ibmruntimes/openj9-openjdk-jdk21#103

RUNNABLE=2 state was removed, and all other states are re-numbered
due to the removal of the RUNNABLE state.

Two new states are added: UNPARKED=9 and YIELDED=11.

Currently, VirtualThread.yield incorrectly consumes the parking
permit. The parking permit should only be consumed when continuing
from VirtualThread.park. The two new states are added to address
this incorrect behaviour.

Signed-off-by: Babneet Singh <[email protected]>
@babsingh
Copy link
Contributor Author

This is to support the fix for https://bugs.openjdk.org/browse/JDK-8321270. The fix is currently in JDK22 and JDK23 (JDK-next). The fix will eventually be ported to JDK21 as per https://bugs.openjdk.org/browse/JDK-8321270.

Commit title -- 8321270: Virtual Thread.yield consumes parking permit

This PR doesn't have ifdefs for JDK22+. So, this PR shouldn't be merged until the fix is ported to JDK21.

ibmruntimes/openj9-openjdk-jdk21#103 backports the fix to JDK21. To avoid any side-effects from JDK-8321270's fix, the backport can wait until the 0.42 and 0.43 targets are released.

If JDK-8321270's fix is ported by the RI to JDK21, then we can just merge this PR and close ibmruntimes/openj9-openjdk-jdk21#103.

fyi @pshipton @tajila

@babsingh babsingh marked this pull request as ready for review December 22, 2023 18:08
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hangshao0 hangshao0 added comp:vm project:loom Used to track Project Loom related work labels Dec 22, 2023
@hangshao0
Copy link
Contributor

Jenkins test sanity.functional,sanity.openjdk aix jdk21 depends ibmruntimes/openj9-openjdk-jdk21#103

@hangshao0
Copy link
Contributor

To avoid any side-effects from JDK-8321270's fix, the backport can wait until the 0.42 and 0.43 targets are released.

Does this really matter ? 0.42 and 0.43 have already branched.

@pshipton
Copy link
Member

Does this really matter ? 0.42 and 0.43 have already branched.

It does make it easier to merge 21.0.2 to 0.43, and any other fixes in the extensions for 0.42/0.43 (and we do need one). I won't merge it now.

I've added it to the 0.43 milestone just in case JDK-8321270 shows up in 21.0.2.

@babsingh
Copy link
Contributor Author

babsingh commented Dec 22, 2023

We should show caution with these changes. Before merging, we should also run the extended.openjdk test suite, which has the JVMTI tests, at least on two platforms (aix and alinux). This is to make sure that these changes have no side-effects (such as timeouts/hangs).

@hangshao0
Copy link
Contributor

Jenkins test extended.openjdk aix jdk21 depends ibmruntimes/openj9-openjdk-jdk21#103

@hangshao0
Copy link
Contributor

Jenkins test extended.openjdk alinux jdk21 depends ibmruntimes/openj9-openjdk-jdk21#103

@hangshao0
Copy link
Contributor

I've added it to the 0.43 milestone just in case JDK-8321270 shows up in 21.0.2.

Does 21.0.2 have the JDK-8321270 fix ?

@hangshao0
Copy link
Contributor

Does 21.0.2 have the JDK-8321270 fix ?

I see this is moved to Release 0.44, so I guess the answer is no.

@babsingh
Copy link
Contributor Author

I see this is moved to Release 0.44, so I guess the answer is no.

Yes, @pshipton plans to merge it in a few days after monitoring the new extension repo changes.

@hangshao0
Copy link
Contributor

OK, I'll let Peter merge this.

@pshipton pshipton merged commit 405abab into eclipse-openj9:master Jan 26, 2024
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants