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

x64 JIT support for virtual threads (Loom) #16002

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

0xdaryl
Copy link
Contributor

@0xdaryl 0xdaryl commented Sep 28, 2022

  • Misc. JIT infrastructural changes to support Loom
  • x64 inc/dec of ownedMonitorCount J9VMThread field on monitor enter/exit
  • x64 inc/dec of callOutCount J9VMThread field for JNI dispatch

Issue: #15175

* thisThreadGetOwnedMonitorCountOffset()
* thisThreadGetCallOutCountOffset()

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
* callOutCount
* ownedMonitorCount

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Required for virtual thread support.

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Only required for hard realtime support, which has been long deprecated.

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Required for virtual thread support.

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Sep 28, 2022

This is essentially the same PR as #15552. The main difference is that the refactoring of one of the monitor enter/exit out of line sequences to remove a seemingly unnecessary label is not done. It turns out that label anchors a register dependency that is required to ensure correct register assignment on the OOL sequence. It was removed in the earlier PR to produce a better instruction sequence. This PR leaves it in and adds a monitor counter update instruction to the OOL path.

This PR will remain a WIP until internal testing is complete.

@0xdaryl
Copy link
Contributor Author

0xdaryl commented Sep 28, 2022

Jenkins test sanity all jdk17,jdk19

@0xdaryl
Copy link
Contributor Author

0xdaryl commented Sep 28, 2022

IBM SDK8 testing passed on 32-bit/64-bit Windows and Linux.

@pshipton
Copy link
Member

pshipton commented Sep 28, 2022

We had some random failures that I assume were caused by the original changes.
I think we should run the sanity.openjdk suite before merging this.

#15977 - sanity.functional jdk17 Windows
#15975 - sanity.openjdk jdk8 Mac
#15974 - extended.functional jdk17 Windows
#15972 - sanity.openjdk jdk11 Mac & Windows
#15971 - extended.functional jdk11 xLinux

@pshipton
Copy link
Member

jenkins test sanity.openjdk xmac,xlinux,win,win32 jdk8,jdk11,jdk17

@pshipton
Copy link
Member

pshipton commented Sep 28, 2022

FYI last nightly build was fine, but there are other changes merged since then in the PR build.
cb8605d...22d152b

https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-JDK19/164/

@pshipton
Copy link
Member

OMR acceptance is also hanging, so the hangs are not caused by this PR.

@pshipton
Copy link
Member

Created #16011

@0xdaryl
Copy link
Contributor Author

0xdaryl commented Sep 29, 2022

Internally I ran sanity.functional, extended.functional, and sanity.system on Linux, macOS, and Windows on JDK 17 and JDK 19 and all were clean.

@0xdaryl 0xdaryl changed the title WIP: x64 JIT support for virtual threads (Loom) x64 JIT support for virtual threads (Loom) Sep 29, 2022
@pshipton
Copy link
Member

pshipton commented Sep 29, 2022

Given the internal testing, and what passed in PR testing, I have no concerns having this merged.

@0xdaryl
Copy link
Contributor Author

0xdaryl commented Sep 29, 2022

@joransiu : Sorry for the churn. The previous PR that you reviewed and merged was reverted due to some issues being found on 32-bit. This PR is largely the same as that, except it avoids some seemingly benign out-of-line instruction refactoring for monitor enter/exit sequences to make placement of the Loom counter updates easier. It turns out there are some register assignment subtleties that rely on the shape of the OOL sequence so in this PR I left the the OOL sequence largely as-is but did add a counter bump for Loom. Full testing on 32-bit and JDK 17 and 19 all looks good. Failures found in this PR testing are attributed to #16011, as Pete mentions above.

Would you mind reviewing and merging again please? Thanks.

@joransiu
Copy link
Member

Given the testing mentioned above, and failures are unrelated to this issue, I'll go ahead and merge.

@joransiu joransiu merged commit 5a8ae64 into eclipse-openj9:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants