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 Continuation direct transition support #15678

Merged
merged 3 commits into from Aug 18, 2022

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Aug 5, 2022

  • Cache jitGPR data in J9VMContinuation
  • Wrap Continuation.execute call to force yield after completion
  • Build Callin frame during first enterContinuation call
  • added helper to copy Continuation data into J9VMThread structure

Closes: #15464

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@fengxue-IS fengxue-IS force-pushed the direct_transition branch 2 times, most recently from 93f4395 to 650f1c2 Compare August 8, 2022 19:16
@fengxue-IS fengxue-IS marked this pull request as ready for review August 8, 2022 19:16
@fengxue-IS fengxue-IS changed the title [WIP] direct transition changes Add Continuation direct transition support Aug 8, 2022
@fengxue-IS fengxue-IS added the project:loom Used to track Project Loom related work label Aug 8, 2022
@fengxue-IS fengxue-IS self-assigned this Aug 8, 2022
runtime/oti/j9nonbuilder.h Show resolved Hide resolved
runtime/vm/ContinuationHelpers.cpp Outdated Show resolved Hide resolved
@tajila
Copy link
Contributor

tajila commented Aug 9, 2022

@fengxue-IS do the ppc jit failures still occur with these changes?

@gacholio
Copy link
Contributor

This PR should also provide the walkContinuationStackFrames wrapper function.

@fengxue-IS fengxue-IS force-pushed the direct_transition branch 2 times, most recently from 397d100 to a86060f Compare August 15, 2022 15:02
@gacholio
Copy link
Contributor

Still need to add the stack walk wrapper.

@fengxue-IS
Copy link
Contributor Author

Per discussion with @LinHu2016, I included part of Lin's code to prepare a stack allocated J9VMThread for stackwalk with data from Continuation object, this helper will be shared by VM and GC.

@gacholio
Copy link
Contributor

The VMHelpers change is not how I would like this done. Please create a wrapper for walkStackFrames as I've described above. It should be a C function, not an inline header function.

Also note that decompilationStack will also need to be swapped like other roots.

@fengxue-IS
Copy link
Contributor Author

The current VMHelper function is what GC would need as they have code that specifically depends on a walkable J9VMThread passed to top level function which eventually would call walkStackFrames.

I can add a separate wrapper that would accept a continuation object and walkState, then stack allocate the J9VMThread/ELS and uses this api to copy the data and calls walkStackFrames in the end

@fengxue-IS
Copy link
Contributor Author

Also note that decompilationStack will also need to be swapped like other roots.

do j2iFrame also need to be swapped?

@gacholio
Copy link
Contributor

do j2iFrame also need to be swapped?

Yes.

@gacholio
Copy link
Contributor

I can add a separate wrapper that would accept a continuation object and walkState, then stack allocate the J9VMThread/ELS and uses this api to copy the data and calls walkStackFrames in the end

Yes, this is what we'll need (think javacore). Will the wrapper be able to share the helper code? I dislike having multiple copies of this logic, particularly split between VM and GC.

@gacholio
Copy link
Contributor

Feel free to remove the ifdefs from the VMHelpers code - we always have a JIT and always have FSD. Certainly don't propagate the ifdefs into the structure definition.

@gacholio
Copy link
Contributor

Actually, the wrapper function should take the J9JavaVM rather than the current J9VMThread (which is NULL is some of the RAS code).

@LinHu2016
Copy link
Contributor

Thanks @fengxue-IS for new walkContinuationStackFrames function, currently in GC there is no precompile option (#if JAVA_SPEC_VERSION >= 19), so could you please add a wrapper for the walkContinuationStackFrames() (the function is for java 19 only)?

@gacholio
Copy link
Contributor

I was under the impression the GC could not use walkContinuationStackFrames in its current form, which is why I'm asking for the more generic helper.

@fengxue-IS
Copy link
Contributor Author

I was under the impression the GC could not use walkContinuationStackFrames in its current form, which is why I'm asking for the more generic helper.

Discussed the logic with @LinHu2016 offline, GC have optimized their design to combine the different hierarchy into a single location on scanning Continuation object which allow them to directly consume the walkContinuationStackFrames API.

I will move code from copyJavaStacksFromContinuation directly into walkContinuationStackFrames and create a new wrapper in VMHelpers that perform the ifdef on Java version GC requires.

@gacholio
Copy link
Contributor

OK, please add the J9JavaVM parameter to it as we need it to initialize the J9VMThread for use in the stack walk, and we can't depend on being able to fetch it from the current thread.

@gacholio
Copy link
Contributor

gacholio commented Aug 17, 2022

Also, please zero initialize the ELS and J9VMThread to 0 at the start (using = { 0 }).

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Aug 17, 2022

OK, please add the J9JavaVM parameter to it as we need it to initialize the J9VMThread for use in the stack walk, and we can't depend on being able to fetch it from the current thread.

We would need currentVMThread regardless to fetch the J9VMContinuation struct from the Continuation object via J9VMJDKINTERNALVMCONTINUATION_VMREF(vmthread, object) macro?

Edit: I guess I can use the J9VMJDKINTERNALVMCONTINUATION_VMREF_VM version which rely on J9JAVAVM_VMTHREAD()

@gacholio
Copy link
Contributor

There should me a matching _VM version of the macro generated, I believe.

@fengxue-IS
Copy link
Contributor Author

@gacholio what about the currentThread that walkStackFrames expect? do I also use the J9JAVAVM_VMTHREAD macro?

@gacholio
Copy link
Contributor

The stack walker can handle the NULL current thread already so you can just pass it through.

@gacholio
Copy link
Contributor

The case we're interested in handling is when javacore generation is signalled on a non-java thread, so in that case asking the VM for the current thread would not work.

All of this is assuming we will eventually want to display continuation stacks in the javacore.

@gacholio
Copy link
Contributor

I'm not sure the macro will actually work in this case unless the GC can also handle a NULL vmThread.

Maybe for now leave the function signature the way it is and add an assert that the current thread is not NULL. We can consider this again when javacore support is added.

@gacholio
Copy link
Contributor

Please squash the commits.

fengxue-IS and others added 3 commits August 17, 2022 16:06
- Cache jitGPR data in J9VMContinuation
- Wrap `Continuation.execute` call to force yield after completion
- Build Callin frame during first enterContinuation call
- Update j9vmcontinuation to include j2iframe and decompstack

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
- prepare a stub J9VMThread with necessary field for stack walk using data
from Continuation object and calls walkStackFrames with stub thread and walkState
- Add wrapper for walkContinuationStackFrames used by GC in VMHelpers

Co-authored-by: Lin Hu <linhu@ca.ibm.com>
Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@gacholio
Copy link
Contributor

jenkins test sanity.functional xlinux jdknext

@gacholio
Copy link
Contributor

jenkins compile win jdk8,jdknext

@gacholio
Copy link
Contributor

Jep425Tests_testVirtualThread_0 failure.

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Aug 18, 2022

@gacholio can you please run the sanity suite against jdk19, I think jdknext is running with jdk20 which might not work as virtualthread is preview for Java 19

@tajila
Copy link
Contributor

tajila commented Aug 18, 2022

jenkins test sanity.functional xlinux jdk19

@gacholio gacholio mentioned this pull request Aug 18, 2022
@gacholio gacholio merged commit b1a406d into eclipse-openj9:master Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit project:loom Used to track Project Loom related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loom: Add support for direct transition to interpreter on continuation entry
5 participants