Skip to content

Add JVMTI_EVENT_MONITOR_WAIT/ED event trigger for virtual threads #21590

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

Merged
merged 2 commits into from
Apr 15, 2025

Conversation

fengxue-IS
Copy link
Contributor

Fixes: #21400

@fengxue-IS
Copy link
Contributor Author

@babsingh please take a look

@fengxue-IS
Copy link
Contributor Author

local testing of the failed test case passed on amac
personal build in progress

@babsingh
Copy link
Contributor

babsingh commented Apr 8, 2025

The top commit in https://github.com/babsingh/openj9/commits/monitorWaitHook has my code review; some of which @keithc-ca has already mentioned above.

Let's not add fixes 21400 in the commit message since the test would need to be excluded before the issue is closed.

#21402 should also pass with these changes, but it still fails.

@fengxue-IS
Copy link
Contributor Author

Let's not add fixes 21400 in the commit message since the test would need to be excluded before the issue is closed.

#21402 should also pass with these changes, but it still fails.

#21402 is still failing because the hook is suppressed by jvmtiHook.c:3846:shouldPostEvent(). I will look into where the the flag for suppression finishes and if we need to re-order the hook trigger point.

@fengxue-IS
Copy link
Contributor Author

I think I've found the root cause here, the call to enter/exitVThreadTransitionCritical in preparePinnedVirtualThreadForMount/Unmount isn't accompanied by the matching call to VM_VMHelpers::virtualThreadHideFrames which cause some jvmti events to be suppressed during hook dispatch.

In order to fix this issue, we must ensure that all event hooks which should be posted for vthreads must occur before or after the critical region. so that the stack/thread data is in a walkable state.

I will re-work this PR to correctly handle the framehide and critical transition region.

@fengxue-IS fengxue-IS marked this pull request as draft April 9, 2025 18:33
@fengxue-IS fengxue-IS force-pushed the monitorWaitHook branch 3 times, most recently from 8e35771 to 0c73455 Compare April 9, 2025 20:05
@fengxue-IS fengxue-IS marked this pull request as ready for review April 9, 2025 22:09
@babsingh
Copy link
Contributor

babsingh commented Apr 10, 2025

@fengxue-IS There is a merge conflict, which needs to be resolved.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Passing to @keithc-ca for review/merge.

@fengxue-IS
Copy link
Contributor Author

@keithc-ca are you okay with this PR or is there anything you would suggest changing?

@fengxue-IS fengxue-IS force-pushed the monitorWaitHook branch 2 times, most recently from 3e05b14 to f72ee8e Compare April 11, 2025 22:17
@fengxue-IS fengxue-IS force-pushed the monitorWaitHook branch 2 times, most recently from 75f3526 to 9cd2de3 Compare April 14, 2025 20:57
Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

Please rebase to avoid implicit merges with other changes (e.g. #21611).

- Move ContendedMonitorEnter before critical region
- Correct virtualThreadHideFrames scope

Signed-off-by: Jack Lu <[email protected]>
@keithc-ca
Copy link
Contributor

Jenkins test sanity zlinux jdk24

@babsingh babsingh merged commit 0ac006d into eclipse-openj9:master Apr 15, 2025
6 checks passed
@keithc-ca
Copy link
Contributor

I was awaiting the results from https://openj9-jenkins.osuosl.org/job/Grinder/4266 which failed.

@babsingh
Copy link
Contributor

babsingh commented Apr 15, 2025

I was awaiting the results from https://openj9-jenkins.osuosl.org/job/Grinder/4266 which failed.

This is a separate issue unrelated to this PR. The test is RI specific; it will need to be updated for OpenJ9. @fengxue-IS has this task in his backlog; he will be creating extension repo PRs to fix it.

09:55:57  MonitorWaited event:
09:55:57  	thread: 0x13e6f40, object: 0x13e6f08, timed_out: false
09:55:57  Thread: 0x13e6f40, name: Debuggee Thread, state(5):  ALIVE RUNNABLE, attrs: virtual daemon
09:55:57  JVMTI Stack Trace for thread Debuggee Thread: frame count: 7
09:55:57   0: java/lang/Object: wait(JI)V
09:55:57   1: java/lang/Object: wait(J)V
09:55:57   2: monitorwaited01Task: run()V
09:55:57   3: java/lang/Thread: runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V
09:55:57   4: java/lang/VirtualThread: run(Ljava/lang/Runnable;)V
09:55:57   5: java/lang/VirtualThread$VThreadContinuation$1: run()V
09:55:57   6: jdk/internal/vm/Continuation: enter(Ljdk/internal/vm/Continuation;)V
09:55:57  
09:55:57  Expected 8 methods in the stack but found 7

@keithc-ca
Copy link
Contributor

That test should have been updated and verified to pass before this was merged.

@babsingh
Copy link
Contributor

babsingh commented Apr 15, 2025

That test should have been updated and verified to pass before this was merged.

The test is excluded. It doesn't make a difference. Fixes can go in asynchronously since the test is excluded.

@keithc-ca
Copy link
Contributor

Yes, the test is excluded, but this claimed to fix the problem, but does not.

@babsingh
Copy link
Contributor

It does fix the issue documented in #21400 by adding support for the JVMTI events. The failure in #21590 (comment) is a separate issue tracked under #21402.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JDK24 JVMTI serviceability MonitorWait monitorwait01
3 participants