openj9
openj9 copied to clipboard
eclipse-openj9
[WIP] Virtual thread support for JVMTI extension functions.
@thallium This is becoming a large PR (1000+ lines). Please create a separate PR for each JVMTI extension function for ease of reviewing and reducing the risk of failures.
When do you expect to finish this work?
Related: #18386
fyi @pshipton @TobiAjila
@babsingh Currently I'm investigating the intermittent test failure of added tests so I should be able to finalize the PR next week.
Actually a lot of the changes are formatting changes and the test code for GetStackTraceExtended and GetThreadListStackTracesExtended are pretty much the same so the actual code to be reviewed is much less.
5x60 tests passed https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/38282/
PR title shouldn't have a full-stop; I have removed it.
The PR description and commit message needs more details:
- which JVMTI extension functions are updated?
- what do the tests do?
- link to the related issue: https://github.com/eclipse-openj9/openj9/issues/18386.
The tests should also have a comment with a high-level description.
There are typos in the commit messages. Correction version:
In each of the tests, a virtual thread runs methodToCompile()
for a relatively large number of iterations to JIT compile the
method. Then, we use the JVMTI Extension API to check if there
are any JITted frames in the stack trace of the virtual thread.
We test this for both mounted and unmounted virtual threads.
@llxia There are two variants of existing tests in jvmtitests/playlist.xml: ^arch.390 and arch.390. Do we need to do the same for the new test?
The new test cmdLineTester_jvmtitests_Java21andUp only uses jvmtitests_Java21andUp.xml that you added. Do we want it to run on arch.390?
The new test cmdLineTester_jvmtitests_Java21andUp only uses jvmtitests_Java21andUp.xml that you added. Do we want it to run on arch.390?
It should be run everywhere. But, existing tests have two targets: one for arch.390 (e.g. cmdLineTester_jvmtitests_nongold) and another for non arch.390 platforms (e.g cmdLineTester_jvmtitests). The only visible difference is that the arch.390 test target is tagged to extended.functional whereas the non arch.390 test target is tagged to sanity.functional. Not sure why this was done. If it doesn't matter anymore, we can just have one test target.
I've updated the test variations to test different GC policies. Do we need to test compressed ref as well?
Do we need to test compressed ref as well?
The current settings have compressed refs enabled.
These changes LGTM. I plan to launch the PR builds. @llxia Do the test changes look good to you?
jenkins test sanity.functional,extended.functional,sanity.openjdk,extended.openjdk win,alinux jdk21
jenkins test extended.functional,sanity.openjdk,extended.openjdk amac jdk22
PR build errors (test code shouldn't be compiled on JDK11):
05:51:28 [javac] /home/jenkins/workspace/Test_openjdk11_j9_extended.functional_s390x_linux_Personal_testList_0/aqa-tests/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/getStackTraceExtended/gste002.java:78: error: cannot find symbol
05:51:28 [javac] Thread t1 = Thread.ofVirtual().name("vthread 1").start(() -> {
05:51:28 [javac] ^
05:51:28 [javac] symbol: method ofVirtual()
05:51:28 [javac] location: class Thread
05:51:28 [javac] /home/jenkins/workspace/Test_openjdk11_j9_extended.functional_s390x_linux_Personal_testList_0/aqa-tests/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/getStackTraceExtended/gste002.java:116: error: cannot find symbol
05:51:28 [javac] try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
05:51:28 [javac] ^
05:51:28 [javac] symbol: method newVirtualThreadPerTaskExecutor()
05:51:28 [javac] location: class Executors
05:51:28 [javac] /home/jenkins/workspace/Test_openjdk11_j9_extended.functional_s390x_linux_Personal_testList_0/aqa-tests/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/getThreadListStackTracesExtended/gtlste002.java:78: error: cannot find symbol
05:51:28 [javac] Thread t1 = Thread.ofVirtual().name("vthread 1").start(() -> {
05:51:28 [javac] ^
05:51:28 [javac] symbol: method ofVirtual()
05:51:28 [javac] location: class Thread
05:51:28 [javac] /home/jenkins/workspace/Test_openjdk11_j9_extended.functional_s390x_linux_Personal_testList_0/aqa-tests/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/getThreadListStackTracesExtended/gtlste002.java:116: error: cannot find symbol
05:51:28 [javac] try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
test code shouldn't be compiled on JDK11
@llxia Could you help with this issue? Which part need to be modified?
See existing tests such as soae001 (Java11andUp). One missing item in runtime/tests/jvmtitests/agent/tests.c is
#if JAVA_SPEC_VERSION >= 21
{ "gste002", gste002, "com.ibm.jvmti.tests.getStackTraceExtended.gste002", "GetStackTraceExtended" },
{ "gtlste002", gtlste002, "com.ibm.jvmti.tests.getThreadListStackTracesExtended.gtlste002", "GetThreadListStackTracesExtended" },
#endif /* JAVA_SPEC_VERSION >= 21 */
@babsingh all the above changes are mostly related to the native functions. We haven't add anything to prevent the Java testing code from compiling. In the playlist.xml we already have:
<versions>
<version>21+</version>
</versions>
but that doesn't seem to be effective. I'm stilling getting the same error in my local testing.
In .../jvmtitests/build.xml, adding <exclude name="... for those files might prevent them from compiling. @llxia for confirming?
playlist.xml controls the execution only. For java compilation, we need to update build.xml.
<if>
<not>
<equals arg1="${JDK_VERSION}" arg2="11" />
</not>
//add additional exclude files
...
https://github.com/eclipse-openj9/openj9/blob/6b03a382a12a2293d1cb4d884c55899859da516e/test/functional/cmdLineTests/jvmtitests/build.xml#L81
LGTM. @llxia I will merge the PR after you complete your review.
https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_extended.openjdk_x86-64_windows_Personal/3
An infra issue similar to https://github.com/eclipse-openj9/openj9/issues/8446 is seen in the above PR build, which is unrelated to this PR. fyi @AdamBrousseau