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.

Sign up for GitHub

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

Jump to bottom

Support yield of pinned VirtualThread in VM code path #21064

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support yield of pinned VirtualThread in VM code path #21064

wants to merge 4 commits into from

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Feb 4, 2025 •
edited by babsingh
Loading

add new code path to intercept enterObjectMonitor function in BytecodeInterpreter.

New helper to inflate all owned monitors and blocking object.
implement takeVirtualThreadListToUnblock API to generate list of virtual threads to unblock.

add check in Object.wait/notify to yield virtual thread if possible.

Adds new -XX:[+|-]YieldPinnedVirtualThreads option

Related: #20942

@fengxue-IS fengxue-IS force-pushed the jep491-3 branch 11 times, most recently from 8ee76cb to c982de9 Compare February 4, 2025 23:48
@fengxue-IS
Copy link
Contributor Author

Updated monitor waiting list to native ref per discussion #20942 (comment), added vthread ref to J9VMContinuation so monitor will know which vthread to to notify.

@fengxue-IS fengxue-IS force-pushed the jep491-3 branch 2 times, most recently from 172fcc7 to 2859911 Compare February 14, 2025 03:04
@babsingh babsingh self-requested a review February 14, 2025 18:43
@babsingh
Copy link
Contributor

The remaining TODOs that are planned to be completed by EOD:

  • Runtime flag to toggle the feature on/off
  • Helper functions for the redundant code
  • Update on functional testing with -Xint / -Xgcpolicy:nogc
  • Comments should be squashed
  • Formatting should be updated to adhere coding standards (Issues noticed: Doxygen function descriptions are incomplete; comments are not formatted properly; typos)

tajila
tajila reviewed Feb 19, 2025
View reviewed changes
@fengxue-IS fengxue-IS force-pushed the jep491-3 branch 2 times, most recently from c5c8769 to 848f757 Compare February 21, 2025 21:47
@babsingh
Copy link
Contributor

babsingh commented Feb 24, 2025 •
edited
Loading

@fengxue-IS Can you squash your commits? My code review changes are in the top commit at https://github.com/babsingh/openj9/commits/jep491-3. If you have any questions about the code review changes, let's set up a call to resolve them quickly. Otherwise, please apply the code review changes while squashing the commits. I was only able to successfully compile after applying the code review changes, so please run some local tests on your end afterward.

@fengxue-IS
Copy link
Contributor Author

@babsingh changes have been squashed and updated following the review suggestions

@babsingh babsingh marked this pull request as ready for review February 24, 2025 19:48
@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk amac jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk alinux jdk24

@babsingh
Copy link
Contributor

@fengxue-IS Not all code is properly ifdef'ed for JDK24:

14:55:47  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/monhelpers.c:228:27: error: no member named 'virtualThreadWaitCount' in 'struct J9ObjectMonitor'
14:55:47                  && (0 != objectMonitor->virtualThreadWaitCount)
14:55:47                           ~~~~~~~~~~~~~  ^
14:55:47  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/monhelpers.c:230:46: error: no member named 'blockedVirtualThreadsMutex' in 'struct J9JavaVM'
14:55:47                          omrthread_monitor_enter(vmStruct->javaVM->blockedVirtualThreadsMutex);
14:55:47                                                  ~~~~~~~~~~~~~~~~  ^
14:55:47  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/monhelpers.c:231:47: error: no member named 'blockedVirtualThreadsMutex' in 'struct J9JavaVM'
14:55:47                          omrthread_monitor_notify(vmStruct->javaVM->blockedVirtualThreadsMutex);
14:55:47                                                   ~~~~~~~~~~~~~~~~  ^
14:55:47  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/monhelpers.c:232:45: error: no member named 'blockedVirtualThreadsMutex' in 'struct J9JavaVM'
14:55:47                          omrthread_monitor_exit(vmStruct->javaVM->blockedVirtualThreadsMutex);
14:55:47                                                 ~~~~~~~~~~~~~~~~  ^

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk amac jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk zlinux jdk24

@babsingh
Copy link
Contributor

jenkins compile win jdk21,jdk24

@babsingh
Copy link
Contributor

JDK21 compilation still fails:

15:49:40  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/ContinuationHelpers.cpp:270:6: error: expected value in expression
15:49:40  #elif
15:49:40       ^
15:49:40  /Users/jenkins/workspace/Build_JDK21_aarch64_mac_Personal/openj9/runtime/vm/ContinuationHelpers.cpp:348:16: error: no member named 'returnState' in 'J9VMContinuation'
15:49:40          continuation->returnState = returnState;
15:49:40          ~~~~~~~~~~~~  ^
15:49:40  2 errors generated.

@babsingh
Copy link
Contributor

@fengxue-IS Can you compile locally or in a personal build to make sure all compilation failures are resolved before the next push?

@babsingh
Copy link
Contributor

I have terminated the JDK21 Windows compile build since it will fail. JDK24 builds might pass; allowed them to continue running.

@fengxue-IS
Add basic yield pinned vthread support
fc83996 
Update Continuation.Pinned enum
Add field refs in vmconstantpool.xml
add helper to update monitor info
add enterObjectMonitor intercept
Add support for Object.wait()
add support for object.notify()
Add reverse link between J9VMContinuation and vthread & remove Object ref in J9ObjectMonitor

Signed-off-by: Jack Lu <[email protected]>
@fengxue-IS
Add runtime option -XX:[+|-]YieldPinnedContinuation
aa26f3f 
Add helper function to reduce duplicate code

Signed-off-by: Jack Lu <[email protected]>
@fengxue-IS
Fix vthread Object.wait/notify monitor operation
fda0972 
Signed-off-by: Jack Lu <[email protected]>
@fengxue-IS
Copy link
Contributor Author

running build locally to verify jdk21 isn't impacted by this change

@fengxue-IS
Copy link
Contributor Author

@babsingh confirmed locally JDK21 compile passed.

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk amac jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk zlinux jdk24

@babsingh
Copy link
Contributor

jenkins compile win jdk21,jdk24

@fengxue-IS
Copy link
Contributor Author

looking into the Skynet failure on mac for JDK21, so far haven't been able to reproduce on xlinux, will try on amac

@babsingh
Copy link
Contributor

babsingh commented Feb 25, 2025 •
edited
Loading

looking into the Skynet failure on mac for JDK21, so far haven't been able to reproduce on xlinux, will try on amac

For the failures, there are core files in the PR builds:

@fengxue-IS Did you take a look at the core files?

The segfault occurs in 17:26:54 Module_base_address=00000001068A0000 Symbol=yieldContinuation. It seems like a new failure. The old failures occurred in swalk.c.

@fengxue-IS @babsingh
Refactor code to address compile/formating issue
5565ea5 
Correct monitorRecords pool allocation
Release VMAccess when waiting for unblocked virtual threads
Move JVM_TakeVirtualThreadListToUnblock code into helper
Rename XX:YieldPinnedContinuation to XX:YieldPinnedVirtualThreads

Co-authored-by: Jack Lu <[email protected]>
Co-authored-by: Babneet Singh <[email protected]>
Signed-off-by: Jack Lu <[email protected]>
@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Feb 25, 2025 •
edited
Loading

Corrected the bug which is causing the crash on Mac, personal build to verify changes in progress

issue is caused by access to recycled J9VMContinuation struct, moved the access code under check to ensure the J9VMContinuation pointer is always valid

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Feb 25, 2025 •
edited
Loading

@babsingh grinder passed, can you restart the PR build

@babsingh
Copy link
Contributor

jenkins test sanity.openjdk amac jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.openjdk zlinux jdk24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tajila tajila tajila left review comments

@babsingh babsingh Awaiting requested review from babsingh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fengxue-IS @babsingh @tajila