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

Implement JVM_VirtualThread[Mount|Unmount|Start|End] #17125

Closed
wants to merge 1 commit into from
Closed

Implement JVM_VirtualThread[Mount|Unmount|Start|End] #17125

wants to merge 1 commit into from

Conversation

thallium
Copy link
Contributor

@thallium thallium commented Apr 6, 2023 •
edited
Loading

  • Move code of APIs of JDK 20 such as JVM_VirtualThreadMountBegin
    to helpers so they can be called by APIs of both JDK 20 and 21
  • Hide frames between mount/unmount begin and mount/unmount end

Fixes #16984

@thallium
Copy link
Contributor Author

thallium commented Apr 6, 2023

@fengxue-IS FYI

@thallium thallium mentioned this pull request Apr 12, 2023
Closed
@thallium thallium force-pushed the jdk21vthreadmount branch 3 times, most recently from 084d53d to 0f06919 Compare May 9, 2023 18:48
@thallium thallium changed the title [WIP] Implement JVM_VirtualThreadMount/JVM_VirtualThreadUnmount Implement JVM_VirtualThreadMount/JVM_VirtualThreadUnmount May 9, 2023
@thallium
Copy link
Contributor Author

thallium commented May 9, 2023

@fengxue-IS can you take a look?

@fengxue-IS fengxue-IS self-requested a review May 9, 2023 20:05
fengxue-IS
fengxue-IS suggested changes May 9, 2023
View reviewed changes
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
@thallium thallium force-pushed the jdk21vthreadmount branch 4 times, most recently from 715e437 to 0c41d0e Compare May 15, 2023 23:03
@thallium
Copy link
Contributor Author

@fengxue-IS Your comments have been addressed.

fengxue-IS
fengxue-IS reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

code looks good, minor format suggestions.
@thallium can you confirm the failure seen in #16346 is fixed

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/oti/VMHelpers.hpp Outdated Show resolved Hide resolved
@dipak-bagadiya
Copy link
Contributor

@fengxue-IS @thallium
"test/hotspot/jtreg/serviceability/jvmti/events/FramePop/framepop02."
test case execution fails.

STDERR:
15:13:29.377 0x272200  j9scar.209    *   ** ASSERTION FAILED ** at /root/openj9-20/openj9-openjdk-jdk20/openj9/runtime/j9vm/javanextvmi.cpp:279: ((isSameOrSuperClassOf((((J9RAMClassRef*)(&((currentThread)->javaVM)->jclConstantPool[(30)]))->value), ((0 != (currentThread)->compressObjectReferences) ? (struct J9Clas

@thallium
Copy link
Contributor Author

@fengxue-IS @thallium "test/hotspot/jtreg/serviceability/jvmti/events/FramePop/framepop02." test case execution fails.

STDERR:
15:13:29.377 0x272200  j9scar.209    *   ** ASSERTION FAILED ** at /root/openj9-20/openj9-openjdk-jdk20/openj9/runtime/j9vm/javanextvmi.cpp:279: ((isSameOrSuperClassOf((((J9RAMClassRef*)(&((currentThread)->javaVM)->jclConstantPool[(30)]))->value), ((0 != (currentThread)->compressObjectReferences) ? (struct J9Clas

@dipak-bagadiya Just want to confirm is this my latest change? I've moved the assertion so it won't be checked during mount/unmount.

@dipak-bagadiya
Copy link
Contributor

@thallium will check with latest commit.

@fengxue-IS fengxue-IS mentioned this pull request May 16, 2023
Closed
@dipak-bagadiya dipak-bagadiya mentioned this pull request May 23, 2023
Closed
@thallium thallium force-pushed the jdk21vthreadmount branch 2 times, most recently from 6130215 to c01be02 Compare May 31, 2023 20:41
keithc-ca
keithc-ca requested changes Jun 1, 2023
View reviewed changes
runtime/redirector/forwarders.m4 Outdated Show resolved Hide resolved
runtime/j9vm/exports.cmake Outdated Show resolved Hide resolved
runtime/j9vm/j9vmnatives.xml Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
@thallium thallium changed the title Implement JVM_VirtualThreadMount/JVM_VirtualThreadUnmount Implement JVM_VirtualThread[Mount|Unmount|Start|End] Jun 1, 2023
babsingh
babsingh requested changes Jun 7, 2023
View reviewed changes
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
keithc-ca
keithc-ca requested changes Jun 9, 2023
View reviewed changes
runtime/j9vm/j9scar.tdf Outdated Show resolved Hide resolved
@keithc-ca
Copy link
Contributor

keithc-ca commented Jun 9, 2023 •
edited
Loading

I merged this into #17514 and ran a small test program that used virtual threads:

19:30:32.718 0x30d900j9vmutil(j9jvmt.16     *   ** ASSERTION FAILED ** at /home/keithc/space/openj9/jdknext/openj9/runtime/util/eventframe.c:87: (!(currentThread->inNative))

Here's an excerpt of the native stack:

4XENATIVESTACK               raiseAssertion+0xf1 (0x00007FC1D7AC6261 [libj9trc29.so+0xe261])
4XENATIVESTACK               doTracePoint+0x965 (0x00007FC1D7AC9EC5 [libj9trc29.so+0x11ec5])
4XENATIVESTACK               javaTrace+0xa0 (0x00007FC1D7AC2010 [libj9trc29.so+0xa010])
4XENATIVESTACK               pushEventFrame+0x189 (0x00007FC1D692D519 [libj9jvmti29.so+0x35519])
4XENATIVESTACK               prepareForEvent+0xef (0x00007FC1D6913F3F [libj9jvmti29.so+0x1bf3f])
4XENATIVESTACK               jvmtiHookVirtualThreadStarted+0xab (0x00007FC1D691530B [libj9jvmti29.so+0x1d30b])
4XENATIVESTACK               J9HookDispatch+0x136 (0x00007FC1DC14F9C6 [libj9hookable29.so+0x19c6])
4XENATIVESTACK               JVM_VirtualThreadStart+0x9e (0x00007FC1DC40086E [libjvm.so+0x2086e])
4XENATIVESTACK               ffi_call_unix64+0x56 (0x00007FC1DC378D06 [libj9vm29.so+0x223d06])
4XENATIVESTACK               ffi_call_int+0x1bc (0x00007FC1DC377CBC [libj9vm29.so+0x222cbc])
4XENATIVESTACK               _ZN37VM_DebugBytecodeInterpreterCompressed3runEP10J9VMThread+0x1da73 (0x00007FC1DC260C63 [libj9vm29.so+0x10bc63])
4XENATIVESTACK               debugBytecodeLoopCompressed+0xd5 (0x00007FC1DC2431E5 [libj9vm29.so+0xee1e5])
4XENATIVESTACK                (0x00007FC1DC3068B2 [libj9vm29.so+0x1b18b2])

Apparently, running under the debugger is significant; the program runs fine otherwise.

@thallium thallium force-pushed the jdk21vthreadmount branch 2 times, most recently from 8605740 to e075bdc Compare June 12, 2023 17:14
@thallium
Copy link
Contributor Author

@keithc-ca The assertion failure should be fixed now by moving acquiring VM access from helpers to JVM_* functions.

keithc-ca
keithc-ca reviewed Jun 12, 2023
View reviewed changes
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
keithc-ca
keithc-ca requested changes Jun 12, 2023
View reviewed changes
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
@thallium
Implement JVM_VirtualThread[Mount|Unmount|Start|End]
ef5bb83 
- Move code of APIs of JDK 20 such as JVM_VirtualThreadMountBegin
  to helpers so they can be called by APIs of both JDK 20 and 21
- Hide frames between mount/unmount begin and mount/unmount end
- Seperate out code that is specific to first mount and last unmount.

Fixes eclipse-openj9#16984

Outlines of related functions:

virtualThread[Mount|Unmount]Begin:
    enterVThreadTransition()
    virtualThreadHideFrames(true)

virtualThread[Mount|Unmount]End:
    virtualThreadHideFrames(false)
    exitVThreadTransition()

JVM_VirtualThreadMount(bool hide):
    if (hide)
	virtualThreadMountBegin()
    else
	virtualThreadMountEnd()
	TRIGGER_J9HOOK_VM_VIRTUAL_THREAD_MOUNT()

JVM_VirtualThreadUnmount(bool hide):
    if (hide)
	TRIGGER_J9HOOK_VM_VIRTUAL_THREAD_UNMOUNT()
	virtualThreadUnmountBegin()
    else
	virtualThreadUnmountEnd()

JVM_VirtualThreadStart()
    virtualThreadMountEnd()
    TRIGGER_J9HOOK_VM_VIRTUAL_THREAD_STARTED()

JVM_VirtualThreadEnd()
    TRIGGER_J9HOOK_VM_VIRTUAL_THREAD_END()
    virtualThreadUnmountBegin()

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@thallium
Copy link
Contributor Author

@keithc-ca Your comments have been addressed.

@keithc-ca keithc-ca mentioned this pull request Jun 13, 2023
Merged
keithc-ca
keithc-ca approved these changes Jun 13, 2023
View reviewed changes
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 don't make any more changes to this; #17514 needs to be rebased upon this so ibmruntimes/openj9-openjdk-jdk#608 and an equivalent change for https://github.com/ibmruntimes/openj9-openjdk-jdk21 can all be merged together.

@ChengJin01 ChengJin01 mentioned this pull request Jun 19, 2023
Merged
@keithc-ca
Copy link
Contributor

Any further comments, @babsingh or @fengxue-IS?

Builds in ibmruntimes/openj9-openjdk-jdk#608 have passed and I expect a similar result in ibmruntimes/openj9-openjdk-jdk21#5 after which I am ready to merge #17514 (which includes this) together with ibmruntimes/openj9-openjdk-jdk#608 and ibmruntimes/openj9-openjdk-jdk21#5.

babsingh
babsingh approved these changes Jun 20, 2023
View reviewed changes
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.

Any further comments, @babsingh or @fengxue-IS?

No. LGTM.

@keithc-ca
Copy link
Contributor

Merged via #17514.

@keithc-ca keithc-ca closed this Jun 20, 2023
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Jul 27, 2023
@babsingh
[JDK21] Enable JVMTI framepop02.java#id1
8c9b877 
eclipse-openj9/openj9#17125 fixes framepop02.java#id1, and it was
embedded in eclipse-openj9/openj9#17514 and merged.

Now, the MethodExitTest failure is being tracked through
eclipse-openj9/openj9#17514.

Closes eclipse-openj9/openj9#16346

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Jul 27, 2023
@babsingh
[JDK21] Enable JVMTI framepop02.java#id1
afc3021 
eclipse-openj9/openj9#17125 fixes framepop02.java#id1, and it was
embedded in eclipse-openj9/openj9#17514 and merged.

Now, the MethodExitTest failure is being tracked through
eclipse-openj9/openj9#17490.

Closes eclipse-openj9/openj9#16346

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Jul 27, 2023
Merged
llxia pushed a commit to adoptium/aqa-tests that referenced this pull request Jul 27, 2023
@babsingh
[JDK21] Enable JVMTI framepop02.java#id1 (#4687)
38d1716 
eclipse-openj9/openj9#17125 fixes framepop02.java#id1, and it was
embedded in eclipse-openj9/openj9#17514 and merged.

Now, the MethodExitTest failure is being tracked through
eclipse-openj9/openj9#17490.

Closes eclipse-openj9/openj9#16346

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca approved these changes

@fengxue-IS fengxue-IS fengxue-IS approved these changes

@babsingh babsingh babsingh approved these changes

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

Successfully merging this pull request may close these issues.

JDK21 requires JVM_VirtualThreadMount/JVM_VirtualThreadUnmount implementations
5 participants
@thallium @dipak-bagadiya @babsingh @keithc-ca @fengxue-IS