Improve linkToVirtual() and linkToInterface() dispatch - #17850
Conversation
|
Requesting review from @vijaysun-omr and @0xdaryl for JIT, and from @gacholio for VM |
|
@babsingh Please look over the VM changes. |
|
Pushed a slight correction to a comment |
|
Rebased to fix the |
There was a problem hiding this comment.
For functional verification, my recommendation is jekins test sanity.functional,extended.functional,sanity.system,sanity.openjdk,extended.openjdk alinux64 jdk17,jdk21 and a compile on Windows. Deliberate typo in jekins -> jenkins to avoid launching the PR builds.
|
@fengxue-IS Can you please also review the MethodHandleNatives and associated changes? |
|
Pushed changes to address review comments |
|
JIT changes look ok after my first pass through. |
mpirvu
left a comment
mpirvu
left a comment
There was a problem hiding this comment.
JITServer related changes look good.
|
jenkins compile win jdk8,jdk21 |
|
jenkins test sanity.functional,extended.functional,sanity.system,sanity.openjdk,extended.openjdk alinux64 jdk17,jdk21 |
|
Taking a look at what's going on with this on AArch64 |
0ee25c4 to
5491aeb
Compare
|
Corrected the number of parameters for |
|
Oh, I missed that tests in the PR builds had been running only on AArch64 to begin with |
|
I'm doing some additional testing. Converted to draft in the meantime |
|
OK, sorry for the confusion here. Because there had been so many failures, all on AArch64, and because I hadn't realized it was running only on AArch64 to begin with, I assumed that I had caused the failures, but after looking through them now I think they're unrelated to my changes. We have these two known issues:
All of the remaining failures were in DatagramChannel, which I think is due to some kind of machine/network problem. Not everything has already been captured verbatim in an issue, but I've done my best to link to similar failures where possible. It seems to me that we don't run extended.openjdk very often.
Additionally, there were no crashes, but I expect that any failures caused by this PR would likely be crashes. |
|
jenkins test sanity.functional,extended.functional,sanity.system,sanity.openjdk,extended.openjdk zlinux jdk17,jdk21 |
|
The test failures look like a GIT issue. I don't know how to restart the jobs without relaunching. |
|
You look at https://openj9-jenkins.osuosl.org/job/Pipeline_Build_Test_JDK17_s390x_linux/1702/console and there are "Restart or Abort" links. I've restarted them. |
|
Test_openjdk21_j9_sanity.functional_s390x_linux_Personal
Test_openjdk21_j9_extended.functional_s390x_linux_Personal
Test_openjdk21_j9_sanity.openjdk_s390x_linux_Personal
Test_openjdk17_j9_extended.openjdk_s390x_linux_Personal:
Test_openjdk21_j9_extended.openjdk_s390x_linux_Personal
The Restart links that Peter mentioned don't seem to be working for me, maybe because the Pipeline_Build_Test_... jobs have finished? For the ones that hit network errors with git, I'll restart them here |
|
Jenkins test sanity.functional,extended.functional,sanity.openjdk zlinux jdk21 |
|
In extended.functional, there was one failure. It's a timeout when testing I believe this is #13722 In sanity.openjdk there was also just one failure, which was a crash in This one looks to be #16659 The other two failing checks are just the extended.openjdk tests that I described previously, since I didn't restart those. |
|
I believe that all of the failures have been accounted for |
|
Are we waiting for @0xdaryl to review? |
|
Ah, yes, I think so |
|
Please fix merge conflicts. Otherwise, looks good to me. |
This code was duplicated in multiple methods of J9::TransformUtil.
- Remove the direct- and non-interface virtual-dispatch cases from linkToInterface() in the VM. They used to be needed, but they've been dead since 425a1f1. - Implement a recognized call transformation for linkToInterface(). The generated IL need only handle actual itable-based dispatch, and it does not introduce any control flow in the IL. This avoids doing a J2I transition whenever the callee is compiled. - Change jitLookupDynamicPublicInterfaceMethod (now used in the above- mentioned transformation) to take the MemberName instead of the interface class and iTable index. There's no need to generate IL to find them, since the helper can do that on its own. Cases where they are constant might get slightly slower, but the right way to improve those cases will be to refine and possibly inline linkToInterface(). - Remove the direct-dispatch case from linkToVirtual() in the VM and in the IL generated by recognized call transformer. That case used to be needed, but like the removed linkToInterface() cases, it has been dead since 425a1f1. - Remove the interface-dispatch case from linkToVirtual() in the VM and in the IL generated by recognized call transformer. MemberName.vmindex is no longer a pointer to the J9JNIMethodID, but is now instead the vTable offset for invokevirtual, the iTable index for invokeinterface, and -1 for invokespecial and invokestatic. The interface dispatch case in linkToVirtual() used to occur whenever vmtarget was inherited from an interface. Now MemberName resolution/initialization detects that scenario and sets vmindex to the correct vTable offset for clazz so that linkToVirtual() can dispatch the call the same way as any other. Note that in combination with the previous point, this means that the linkToVirtual() recognized call transformation no longer introduces control flow into the IL. - Refine linkToVirtual() calls in inliner and method handle transformer even when vmtarget has been inherited from an interface. Virtual calls to such methods can occur naturally in the bytecode, and the compiler is able to deal with them. - Delete jitLookupDynamicInterfaceMethod, since it's no longer used.
5491aeb to
830016e
Compare
|
Rebased to fix the merge conflict with |
| TR_ASSERT(haveAccess(), "jniMethodIdFromMemberName requires VM access"); | ||
| return (J9JNIMethodID *)J9OBJECT_U64_LOAD(vmThread(), memberName, vmThread()->javaVM->vmindexOffset); | ||
| } | ||
| *out = {}; |
There was a problem hiding this comment.
This doesn't compile on z/OS:
13:10:07 "/jenkins/workspace/Build_JDK17_s390x_zos_Personal/openj9/runtime/compiler/env/VMJ9.cpp", line 4764.11: CCN5063 (S) The text "{" is unexpected.
13:10:07 CCN0793(I) Compilation failed for file /jenkins/workspace/Build_JDK17_s390x_zos_Personal/openj9/runtime/compiler/env/VMJ9.cpp. Object file not created.
13:10:07 runtime/compiler/CMakeFiles/j9jit.dir/build.make:960: recipe for target 'runtime/compiler/CMakeFiles/j9jit.dir/env/VMJ9.cpp.o' failed
13:10:07 make[6]: *** [runtime/compiler/CMakeFiles/j9jit.dir/env/VMJ9.cpp.o] Error 12
@jdmpapin Is there another way to say this that will be accepted by older compilers?
There was a problem hiding this comment.
The text "{" is unexpected.
suggests that we might get the same error from {0}. I think {} should work as an initializer though. I'll try this:
MemberNameMethodInfo zero = {};
*out = zero;
and of course check to make sure that it compiles on z/OS
There was a problem hiding this comment.
It looks like it should work to use
MemberNameMethodInfo zero = {0};
With {}, the build compiler complained that zero was potentially uninitialized
My personal z/OS build failed for a reason I don't understand. I think it's unrelated, but I've asked for some help confirming that my change builds. I'll open a PR as soon as I get confirmation
9 participants
Remove the direct- and non-interface virtual-dispatch cases from
linkToInterface()in the VM. They used to be needed, but they've been dead since 425a1f1.Implement a recognized call transformation for
linkToInterface(). The generated IL need only handle actual itable-based dispatch, and it does not introduce any control flow in the IL. This avoids doing a J2I transition whenever the callee is compiled.Change
jitLookupDynamicPublicInterfaceMethod(now used in the above-mentioned transformation) to take theMemberNameinstead of the interface class and iTable index. There's no need to generate IL to find them, since the helper can do that on its own. Cases where they are constant might get slightly slower, but the right way to improve those cases will be to refine and possibly inlinelinkToInterface().Remove the direct-dispatch case from
linkToVirtual()in the VM and in the IL generated by recognized call transformer. That case used to be needed, but like the removedlinkToInterface()cases, it has been dead since 425a1f1.Remove the interface-dispatch case from
linkToVirtual()in the VM and in the IL generated by recognized call transformer.MemberName.vmindexis no longer a pointer to theJ9JNIMethodID, but is now instead the vTable offset forinvokevirtual, the iTable index forinvokeinterface, and -1 forinvokespecialandinvokestatic. The interface dispatch case inlinkToVirtual()used to occur whenevervmtargetwas inherited from an interface. NowMemberNameresolution/initialization detects that scenario and setsvmindexto the correct vTable offset forclazzso thatlinkToVirtual()can dispatch the call the same way as any other. Note that in combination with the previous point, this means that thelinkToVirtual()recognized call transformation no longer introduces control flow into the IL.Refine
linkToVirtual()calls in inliner and method handle transformer even whenvmtargethas been inherited from an interface. Virtual calls to such methods can occur naturally in the bytecode, and the compiler is able to deal with them.Delete
jitLookupDynamicInterfaceMethod, since it's no longer used.