Improve linkToVirtual() and linkToInterface() dispatch #17850

jdmpapin · 2023-07-24T20:01:31Z

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.

jdmpapin · 2023-07-25T22:40:20Z

Requesting review from @vijaysun-omr and @0xdaryl for JIT, and from @gacholio for VM

runtime/codert_vm/arm64nathelp.m4

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

gacholio · 2023-07-26T16:50:18Z

@babsingh Please look over the VM changes.

jdmpapin · 2023-07-26T16:59:56Z

Pushed a slight correction to a comment

gacholio

Looks good to me.

jdmpapin · 2023-07-26T19:33:24Z

Rebased to fix the CommunicationStream::MINOR_NUMBER conflict

babsingh

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.

runtime/oti/j9nonbuilder.h

runtime/util/hshelp.c

runtime/vm/BytecodeInterpreter.hpp

runtime/util/hshelp.c

runtime/oti/util_api.h

babsingh · 2023-07-27T20:18:20Z

@fengxue-IS Can you please also review the MethodHandleNatives and associated changes?

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

runtime/util/hshelp.c

jdmpapin · 2023-07-28T21:16:00Z

Pushed changes to address review comments

0xdaryl · 2023-07-29T01:38:37Z

JIT changes look ok after my first pass through.

runtime/compiler/control/JITClientCompilationThread.cpp

babsingh

LGTM.

mpirvu

JITServer related changes look good.

gacholio · 2023-08-02T12:55:15Z

jenkins compile win jdk8,jdk21

gacholio · 2023-08-02T13:23:09Z

jenkins test sanity.functional,extended.functional,sanity.system,sanity.openjdk,extended.openjdk alinux64 jdk17,jdk21

gacholio · 2023-08-02T17:19:57Z

There are a bunch of failures:

https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_extended.openjdk_aarch64_linux_Personal/1/tapTestReport/

https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_extended.openjdk_aarch64_linux_Personal/3/tapTestReport/

jdmpapin · 2023-08-02T18:31:54Z

Taking a look at what's going on with this on AArch64

jdmpapin · 2023-08-02T19:23:37Z

Corrected the number of parameters for jitLookupDynamicPublicInterfaceMethod in *nathelp.m4. I looked for changes specific to AArch64, and the only such change was in arm64nathelp.m4, where I noticed that the parameter count was wrong. Since it was that way on every platform, I'm surprised it didn't cause more widespread failures, but I'm hoping anyway that it was the cause of the AArch64 failures in the PR build

jdmpapin · 2023-08-02T19:29:47Z

Oh, I missed that tests in the PR builds had been running only on AArch64 to begin with

jdmpapin · 2023-08-02T19:35:32Z

I'm doing some additional testing. Converted to draft in the meantime

jdmpapin · 2023-08-03T17:59:24Z

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:

security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java.AmazonCA: revoked certificate
jdk_security_infra_0_FAILED RuntimeException: TEST FAILED: unexpected status of EE certificate #17552
javax/rmi/ssl/SSLSocketParametersTest.java.SSLSocketParametersTest: java.rmi.NoSuchObjectException: test problem
AIX jdk8_openj9-0.26.0-m1 : javax/rmi/ssl/SSLSocketParametersTest.sh.SSLSocketParametersTest : java.rmi.NoSuchObjectException #12168

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.

java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java.MulticastSendReceiveTests: Expected message not recieved.
openjdk xlinux socket failures #8899
java/nio/channels/DatagramChannel/Promiscuous.java.Promiscuous: Expected message not received.
Similar failure with MulticastSocket: jdk_net_0_FAILED java/net/MulticastSocket/Promiscuous.java - RuntimeException: 0.0.0.0/0.0.0.0:36641: Expected message not received, Receive timed out #13815
java/nio/channels/DatagramChannel/PromiscuousIPv6.java.PromiscuousIPv6: Expected message not received.
I couldn't find a prior failure, but this must be similar to Promiscuous.java.
java/nio/channels/DatagramChannel/AdaptorMulticasting.java.AdaptorMulticasting: timeout
Failed (unsure if timeout) in zLinux : java/nio/channels/AsynchronousFileChannel/Lock.java - IOException: Resource deadlock avoided #13018 (comment)
java/nio/channels/DatagramChannel/Loopback.java.Loopback: timeout
Failed (unsure if timeout) in zLinux : java/nio/channels/AsynchronousFileChannel/Lock.java - IOException: Resource deadlock avoided #13018 (comment)

Additionally, there were no crashes, but I expect that any failures caused by this PR would likely be crashes.

gacholio · 2023-08-03T19:16:03Z

jenkins test sanity.functional,extended.functional,sanity.system,sanity.openjdk,extended.openjdk zlinux jdk17,jdk21

gacholio · 2023-08-03T20:33:40Z

The test failures look like a GIT issue. I don't know how to restart the jobs without relaunching.

pshipton · 2023-08-03T20:39:01Z

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.

jdmpapin · 2023-08-04T16:06:42Z

Test_openjdk21_j9_sanity.functional_s390x_linux_Personal

testList_0 passed
testList_1 failed to fetch origin/pr/17850/merge ("the remote end hung up unexpectedly")

Test_openjdk21_j9_extended.functional_s390x_linux_Personal

testList_0 passed
testList_1 failed to fetch again

Test_openjdk21_j9_sanity.openjdk_s390x_linux_Personal

failed to fetch

Test_openjdk17_j9_extended.openjdk_s390x_linux_Personal:

security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java.AmazonCA: certificate revoked (as before)
com/sun/jndi/dns/ConfigTests/Timeout.java.Timeout: DNS error [... No route to host]

Test_openjdk21_j9_extended.openjdk_s390x_linux_Personal

AmazonCA again
SSLSocketParametersTest: java.rmi.NoSuchObjectException: no such object in table (as before)

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

jdmpapin · 2023-08-04T16:08:56Z

Jenkins test sanity.functional,extended.functional,sanity.openjdk zlinux jdk21

jdmpapin · 2023-08-09T01:22:09Z

In extended.functional, there was one failure. It's a timeout when testing -XX:+ExitOnOutOfMemoryError:

Testing: Verify -XX:+ExitOnOutOfMemoryError behaves as expected with OutOfMemoryError
Test start time: 2023/08/04 20:17:28 Coordinated Universal Time
Running command: "/home/jenkins/workspace/Test_openjdk21_j9_extended.functional_s390x_linux_Personal_testList_0/openjdkbinary/j2sdk-image/bin/java"   -Xdump -Xmx64m -XX:+ExitOnOutOfMemoryError -cp "/home/jenkins/workspace/Test_openjdk21_j9_extended.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/cmdLineTest_J9tests/cmdLineTest_J9tests.jar" OnOutOfMemoryErrorTest
Time spent starting: 1 milliseconds
***[TEST INFO 2023/08/04 20:27:28] ProcessKiller detected a timeout after 600000 milliseconds!***

I believe this is #13722

In sanity.openjdk there was also just one failure, which was a crash in OOMEInAQS:

[2023-08-04T22:18:40.450Z] JVMDUMP013I Processed dump event "systhrow", detail "java/lang/OutOfMemoryError".
[2023-08-04T22:18:40.450Z] Exception in thread "Attach API wait loop" java/lang/OutOfMemoryError: Java heap space
[2023-08-04T22:18:40.450Z] 	at java/lang/Throwable.printStackTrace (java.base@21-internal/Throwable.java:554)
[2023-08-04T22:18:40.450Z] 	at java/lang/Throwable.printStackTraceHelper (java.base@21-internal/Throwable.java:338)
[2023-08-04T22:18:40.450Z] 	at java/lang/Throwable.printStackTrace (java.base@21-internal/Throwable.java:307)
[2023-08-04T22:18:40.450Z] 	at java/lang/Throwable.printStackTrace (java.base@21-internal/Throwable.java:250)
[2023-08-04T22:18:40.450Z] STATUS:Passed.
[2023-08-04T22:18:40.450Z] Unhandled exception
[2023-08-04T22:18:40.450Z] Type=Segmentation error vmState=0x00040000

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.

jdmpapin · 2023-08-09T18:24:37Z

I believe that all of the failures have been accounted for

gacholio · 2023-08-09T18:26:02Z

Are we waiting for @0xdaryl to review?

jdmpapin · 2023-08-09T18:28:13Z

Ah, yes, I think so

0xdaryl · 2023-08-14T09:53:06Z

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.

jdmpapin · 2023-08-14T17:33:45Z

Rebased to fix the merge conflict with CommunicationStream::MINOR_NUMBER

Range diff

keithc-ca · 2023-09-06T17:53:35Z

runtime/compiler/env/VMJ9.cpp

-   TR_ASSERT(haveAccess(), "jniMethodIdFromMemberName requires VM access");
-   return (J9JNIMethodID *)J9OBJECT_U64_LOAD(vmThread(), memberName, vmThread()->javaVM->vmindexOffset);
-   }
+   *out = {};


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?

Should it be {0} perhaps?

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

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

jdmpapin added comp:vm comp:jit labels Jul 24, 2023

jdmpapin mentioned this pull request Jul 25, 2023

Consider dropping HCR extensions? #17857

Open

jdmpapin requested review from 0xdaryl, gacholio and vijaysun-omr July 25, 2023 22:39

gacholio reviewed Jul 26, 2023

View reviewed changes

runtime/codert_vm/arm64nathelp.m4 Show resolved Hide resolved

jdmpapin commented Jul 26, 2023

View reviewed changes

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp Outdated Show resolved Hide resolved

gacholio requested a review from babsingh July 26, 2023 16:55

jdmpapin force-pushed the linkto-virt-if branch from 28ffeb1 to 83f33cc Compare July 26, 2023 16:57

gacholio approved these changes Jul 26, 2023

View reviewed changes

jdmpapin force-pushed the linkto-virt-if branch from 83f33cc to 9a11ad2 Compare July 26, 2023 19:25

babsingh reviewed Jul 27, 2023

View reviewed changes

babsingh requested a review from fengxue-IS July 27, 2023 20:18

fengxue-IS reviewed Jul 27, 2023

View reviewed changes

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp Outdated Show resolved Hide resolved

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp Outdated Show resolved Hide resolved

runtime/util/hshelp.c Outdated Show resolved Hide resolved

jdmpapin force-pushed the linkto-virt-if branch from 9a11ad2 to 0ee25c4 Compare July 28, 2023 21:14

vijaysun-omr reviewed Jul 29, 2023

View reviewed changes

runtime/compiler/control/JITClientCompilationThread.cpp Show resolved Hide resolved

vijaysun-omr approved these changes Jul 29, 2023

View reviewed changes

babsingh approved these changes Jul 31, 2023

View reviewed changes

mpirvu approved these changes Aug 2, 2023

View reviewed changes

jdmpapin force-pushed the linkto-virt-if branch from 0ee25c4 to 5491aeb Compare August 2, 2023 19:17

jdmpapin marked this pull request as draft August 2, 2023 19:34

jdmpapin marked this pull request as ready for review August 3, 2023 17:59

jdmpapin added 2 commits August 14, 2023 11:43

Extract method to find or fabricate a shadow for MemberName.vmtarget

8ce0584

This code was duplicated in multiple methods of J9::TransformUtil.

jdmpapin force-pushed the linkto-virt-if branch from 5491aeb to 830016e Compare August 14, 2023 17:28

gacholio merged commit 1b26a8c into eclipse-openj9:master Aug 14, 2023

keithc-ca reviewed Sep 6, 2023

View reviewed changes

pshipton mentioned this pull request Mar 15, 2024

Unexpected system GCs with Semeru 17.0.10 #19153

Closed

jdmpapin mentioned this pull request Sep 9, 2024

Remove outdated comment and logic from refineMethodHandleLinkTo() #20093

Merged

Improve linkToVirtual() and linkToInterface() dispatch #17850

Improve linkToVirtual() and linkToInterface() dispatch #17850

Conversation

jdmpapin commented Jul 24, 2023

jdmpapin commented Jul 25, 2023

gacholio commented Jul 26, 2023

jdmpapin commented Jul 26, 2023

gacholio left a comment

Choose a reason for hiding this comment

jdmpapin commented Jul 26, 2023

babsingh left a comment • edited Loading

Choose a reason for hiding this comment

babsingh commented Jul 27, 2023

jdmpapin commented Jul 28, 2023

0xdaryl commented Jul 29, 2023

babsingh left a comment

Choose a reason for hiding this comment

mpirvu left a comment

Choose a reason for hiding this comment

gacholio commented Aug 2, 2023

gacholio commented Aug 2, 2023

gacholio commented Aug 2, 2023

jdmpapin commented Aug 2, 2023

jdmpapin commented Aug 2, 2023

jdmpapin commented Aug 2, 2023

jdmpapin commented Aug 2, 2023

jdmpapin commented Aug 3, 2023 • edited Loading

gacholio commented Aug 3, 2023

gacholio commented Aug 3, 2023

pshipton commented Aug 3, 2023

jdmpapin commented Aug 4, 2023

jdmpapin commented Aug 4, 2023

jdmpapin commented Aug 9, 2023 • edited Loading

jdmpapin commented Aug 9, 2023

gacholio commented Aug 9, 2023

jdmpapin commented Aug 9, 2023

0xdaryl commented Aug 14, 2023

jdmpapin commented Aug 14, 2023

keithc-ca Sep 6, 2023

Choose a reason for hiding this comment

gacholio Sep 6, 2023

Choose a reason for hiding this comment

jdmpapin Sep 6, 2023

Choose a reason for hiding this comment

jdmpapin Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

jdmpapin Sep 7, 2023

Choose a reason for hiding this comment

babsingh left a comment •

edited

Loading

jdmpapin commented Aug 3, 2023 •

edited

Loading

jdmpapin commented Aug 9, 2023 •

edited

Loading

jdmpapin Sep 7, 2023 •

edited

Loading