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

Add JVMTI VirtualThread Mount and Unmount Events #16501

Merged

Conversation

dipak-bagadiya
Copy link
Contributor

@dipak-bagadiya dipak-bagadiya commented Jan 2, 2023

JVMTI VirtualThreadMount and VirtualThreadUnMount events are part of
the JVMTI Extension API which is not covered by the JVMTI
documentation.

The following JVMTI extension events are implemented:
-VirtualThreadmount: Trigger/Hook the Virtual thread mount event
-VirtualThreadUnmount: Trigger/Hook the Virtual thread unmount event

Related: #16167

Signed-off-by: Dipak Bagadiya dipak.bagadiya@ibm.com

@dipak-bagadiya
Copy link
Contributor Author

@babsingh requesting you to please review it.

@babsingh
Copy link
Contributor

babsingh commented Jan 3, 2023

The copyright dates in all files will change to , 2023. E.g.

 * Copyright (c) 1991, 2023 IBM Corp. and others

@babsingh
Copy link
Contributor

babsingh commented Jan 3, 2023

  • eclipsefdn/eca failure: Commits need to be signed off using the Github ID associated with the Eclipse ECN.
  • There are three commits. There should only be one commit. Something went wrong during rebase. A simple fix is to recreate the branch and cherry-pick the single commit.

@babsingh
Copy link
Contributor

babsingh commented Jan 3, 2023

From @dipak-bagadiya

After executing the test case:

make test TEST="jtreg:test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadTest" JTREG="JAVA_OPTIONS=--enable-preview -Dvm.continuations=true;VERBOSE=all"

The below error is seen:

in jvmtiEventManagement event_type: 48
error in JVMTI SetEventNotificationMode: 102

Potential fix

We have to modify jvmtiSetEventNotificationMode to handle JVMTI extension events. @dipak-bagadiya Suggest a solution by looking at its implementation/code.

@babsingh
Copy link
Contributor

babsingh commented Jan 3, 2023

jvmtiSetEventNotificationMode: Suggest a solution by looking at its implementation/code.

  1. VIRTUAL_THREAD_[MOUNT|UNMOUNT] events require the can_support_virtual_threads. So, these events need to be included in this switch case:
    #if JAVA_SPEC_VERSION >= 19
    case JVMTI_EVENT_VIRTUAL_THREAD_START:
    case JVMTI_EVENT_VIRTUAL_THREAD_END:
    ENSURE_CAPABILITY(env, can_support_virtual_threads);
    break;
    #endif /* JAVA_SPEC_VERSION >= 19 */
  2. An error is thrown based on the J9JVMTI_LOWEST_EVENT and J9JVMTI_HIGHEST_EVENT. This range needs to be updated to include all JVMTI extension events. See the implementation of setEventNotificationMode for the error-handling code.
    rc = setEventNotificationMode(j9env, currentThread, mode, event_type, event_thread, J9JVMTI_LOWEST_EVENT, J9JVMTI_HIGHEST_EVENT);

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.

Note: This is a first review pass. I will review it again after the current feedback is addressed.

debugtools/DDR_VM/data/superset-constants.dat Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/oti/j9consts.h Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Show resolved Hide resolved
@dipak-bagadiya dipak-bagadiya force-pushed the jvmti-MountUnmout-fix branch 4 times, most recently from ba822d5 to 94969e6 Compare January 19, 2023 15:24
@dipak-bagadiya dipak-bagadiya marked this pull request as ready for review January 19, 2023 15:25
@dipak-bagadiya dipak-bagadiya changed the title added JVMTI VirtualThread Mount and UnMount Events. Add JVMTI VirtualThread Mount and Unmount Events Jan 19, 2023
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.

Second review pass.

runtime/include/ibmjvmti.h Outdated Show resolved Hide resolved
runtime/nls/j9ti/jvmti.nls Outdated Show resolved Hide resolved
runtime/nls/j9ti/jvmti.nls Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiExtensionMechanism.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiExtensionMechanism.c Outdated Show resolved Hide resolved
runtime/nls/j9ti/jvmti.nls Outdated Show resolved Hide resolved
runtime/nls/j9ti/jvmti.nls Show resolved Hide resolved
runtime/oti/j9vm.hdf Outdated Show resolved Hide resolved
runtime/oti/j9vm.hdf Outdated Show resolved Hide resolved
@dipak-bagadiya dipak-bagadiya force-pushed the jvmti-MountUnmout-fix branch 2 times, most recently from 8577f12 to d3c96a5 Compare January 20, 2023 18:03
@babsingh
Copy link
Contributor

jenkins test sanity zlinux jdk19

@babsingh
Copy link
Contributor

jenkins compile win jdk8,jdk19

@babsingh
Copy link
Contributor

This PR LGTM. @gacholio Can you please review this PR for any VM access issues?

runtime/oti/j9vm.hdf Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

Can you please review this PR for any VM access issues?

I see no VM access problems (the JVMTI event code is designed to be tolerant of whatever state VM access is in).

babsingh added a commit to babsingh/openj9-openjdk-jdk19 that referenced this pull request Feb 1, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk19 that referenced this pull request Feb 1, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk20 that referenced this pull request Feb 1, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk that referenced this pull request Feb 1, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Copy link
Contributor

babsingh commented Feb 1, 2023

Updated tests across JDK19+ extension repos to use extension_event_index:

For this PR's commit, reverted the numbering for extension events in the top commit of https://github.com/babsingh/openj9/commits/jvmti_extension_funcs. See the changes to runtime/oti/jvmtiInternal.h.

@dipak-bagadiya Can you please apply the above runtime/oti/jvmtiInternal.h changes to this PR, and also verify the new test changes?

@dipak-bagadiya dipak-bagadiya force-pushed the jvmti-MountUnmout-fix branch 2 times, most recently from f969307 to 4c250a4 Compare February 1, 2023 18:10
@gacholio
Copy link
Contributor

gacholio commented Feb 1, 2023

Is currentThread->threadObject the virtual thread when the mount/unmount hooks are fired?

Assuming the answer is yes, please document this in the hook declarations.

@dipak-bagadiya
Copy link
Contributor Author

@babsingh added the changes of "runtime/oti/jvmtiInternal.h" to this PR and verified the test with the updated test module.

@dipak-bagadiya
Copy link
Contributor Author

currentThread->threadObject
@gacholio
Should I add or document the currentThread->threadObject in j9vm.hdf? 

@gacholio
Copy link
Contributor

gacholio commented Feb 2, 2023

Should I add or document the currentThread->threadObject in j9vm.hdf?

Please do.

JVMTI VirtualThreadMount and VirtualThreadUnMount events are part of
the JVMTI Extension API which is not covered by the JVMTI
documentation.

The following JVMTI extension events are implemented:
-VirtualThreadmount: Trigger/Hook the Virtual thread mount event
-VirtualThreadUnmount: Trigger/Hook the Virtual thread unmount event

Fixes: eclipse-openj9#16167

Signed-off-by: Dipak Bagadiya dipak.bagadiya@ibm.com
@dipak-bagadiya
Copy link
Contributor Author

currentThread->threadObject

@gacholio added currentThread->threadObject at the point of hook declarations in j9vm.hdf

@gacholio
Copy link
Contributor

gacholio commented Feb 2, 2023

jenkins test sanity zlinux jdk19

@gacholio
Copy link
Contributor

gacholio commented Feb 2, 2023

jenkins compile win jdk8

@gacholio gacholio merged commit 114c390 into eclipse-openj9:master Feb 2, 2023
babsingh added a commit to babsingh/openj9-openjdk-jdk that referenced this pull request Feb 2, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk that referenced this pull request Feb 2, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk that referenced this pull request Feb 7, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk20 that referenced this pull request Feb 7, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9-openjdk-jdk19 that referenced this pull request Feb 7, 2023
Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
llxia pushed a commit to adoptium/aqa-tests that referenced this pull request Feb 8, 2023
The below tests successfully pass with the implementation of
VirtualThread[M|Unm]ount and Get[Virtual|Carrier]Thread:

- BreakpointInYieldTest.java
- NullAsCurrentThreadTest.java
- VThreadUnsupportedTest.java
- GetStackTraceSuspendedStressTest.java
- RawMonitorTest.java

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16518

Signed-off-by: Dipak Bagadiya <dipak.bagadiya@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants