Disable use of method handle for core reflection in JDK21 #20159

r30shah · 2024-09-12T14:31:02Z

In JEP 416, core reflection was re-implemented with Method Handles with the purpose of simplifying adding new language features. While analyzing performance of workload based on large enterprise based application, I observed that this was causing visible performance degradation. Disabling the use of direct method handles for reflection calls while we are working on improving the performance with direct method handles.

r30shah · 2024-09-12T14:32:12Z

FYI @TobiAjila / @joransiu

jcl/src/java.base/share/classes/java/lang/System.java

keithc-ca · 2024-09-13T13:45:34Z

The commit message and the description here should remove "and above" from "JDK21 and above".

r30shah · 2024-09-13T17:49:51Z

@keithc-ca I have removed WIP from the title and also made changes as you suggested. This is ready for review.

keithc-ca · 2024-09-13T19:50:26Z

Jenkins test sanity,extended alinux jdk21

keithc-ca · 2024-09-16T15:23:36Z

Please review and address the test failure.

r30shah · 2024-09-16T17:24:23Z

Looking at the failure in the build, I think it is because of the changes made in db9a1f6#diff-064f3efdb5a6bf22c75743e2449c189806f8e5a2efa292df7d5c3c0dd7f48eb6 to adapt test_getCallerClass_Helper_Reflection_fromBootExtWithAnnotation to match Method Handle frames.

That PR also switches the core reflection to direct method handle.
@JasonFengJ9 Can you share more information about the PR if you remember ? We are trying to switch to use of old reflection for JDK21 as mentioned in the commit description.

JasonFengJ9 · 2024-09-16T18:42:16Z

Looking at the failure in the build, I think it is because of the changes made in db9a1f6#diff-064f3efdb5a6bf22c75743e2449c189806f8e5a2efa292df7d5c3c0dd7f48eb6 to adapt test_getCallerClass_Helper_Reflection_fromBootExtWithAnnotation to match Method Handle frames.

@r30shah from the code commit history, the change in question was related to

openj9/test/functional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java

Lines 37 to 46 in 0985ff3

    
           private static boolean isSecurityFrameOrInjectedInvoker(Class<?> cls) { 
        
           	boolean rc = "java.lang.invoke.SecurityFrame" == cls.getName(); 
        
           	if (!rc) { 
        
           		String injectedInvoker; 
        
           		if (VersionCheck.major() < 15) { 
        
           			injectedInvoker = ReflectionMHTests.class.getPackageName() + ".InjectedInvoker/"; 
        
           		} else { 
        
           			injectedInvoker = ReflectionMHTests.class.getName() + "$$InjectedInvoker/"; 
        
           		} 
        
           		rc = cls.getName().startsWith(injectedInvoker);

Does this PR change InjectedInvoker?

r30shah · 2024-09-16T18:50:47Z

Does this PR change InjectedInvoker?

It does not, it switches to old reflection instead of method handles. The change that I was asking about was specific to test test_getCallerClass_Helper_Reflection_fromBootExtWithAnnotation that is failing with -Djdk.reflect.useDirectMethodHandle=false. The test prior to the change mentioned in previous comment looked like this, which would pass with -Djdk.reflect.useDirectMethodHandle=false as caller class would be ReflectionMHTests.class. Looking at the new change in the test, for 18 and up, I see that the test was changed to expect the frames using method handles.

JasonFengJ9 · 2024-09-16T19:11:14Z

The test prior to the change mentioned in previous comment looked like this, which would pass with -Djdk.reflect.useDirectMethodHandle=false as caller class would be ReflectionMHTests.class. Looking at the new change in the test, for 18 and up, I see that the test was changed to expect the frames using method handles.

The test was based on the DEFAULT_CONFIG.useDirectMethodHandle.
I don't think -Djdk.reflect.useDirectMethodHandle=false was validated when the change was introduced.
The test might have to be adjusted accordingly.

r30shah · 2024-09-16T19:45:55Z

I have made change to failing test to also check if the useDirectMethodHandle property is set to true to call the new changes added in db9a1f6#diff-064f3efdb5a6bf22c75743e2449c189806f8e5a2efa292df7d5c3c0dd7f48eb6 and with that the test is passing.

root@zhost1:~/testDMH/test/openj9/test/TKG# /root/testDMH/test/jdk/bin/java -Djdk.reflect.useDirectMethodHandle=false -Xdump --add-exports=java.base/com.ibm.oti.vm=ALL-UNNAMED --add-exports java.base/jdk.internal.reflect=ALL-UNNAMED -Xbootclasspath/a:/root/testDMH/test/openj9/test/TKG/../../jvmtest/functional/cmdline_options_testresources/cmdlinetestresources.jar com.ibm.j9.getcallerclass.TestGroup bootClassLoader
test_getCallerClass_fromBootExtWithAnnotation: PASSED
test_ensureCalledFromBootstrapClass_fromBootWithAnnotation: PASSED
test_getCallerClass_Helper_Reflection_fromBootExtWithAnnotation: PASSED: return com.ibm.j9.getcallerclass.ReflectionMHTests
test_getCallerClass_Direct_Reflection_fromBootExtClassLoader: PASSED
test_getCallerClass_Helper_MethodHandle_fromBootExtWithAnnotation: PASSED: return com.ibm.j9.getcallerclass.ReflectionMHTests$$InjectedInvoker/0x0000000000000000
test_getCallerClass_Direct_MethodHandle_fromBootExtClassLoader: PASSED
test_getCallerClass_MethodHandle_ArgumentHelper: PASSED: return com.ibm.j9.getcallerclass.ReflectionMHTests$$InjectedInvoker/0x0000000000000000
test_getCallerClass_fromBootExtWithoutAnnotation: PASSED
test_ensureCalledFromBootstrapClass_fromBootExtWithoutAnnotation: PASSED
All Tests Completed and Passed

@JasonFengJ9 Can you please review last fix I pushed into the PR?

JasonFengJ9 · 2024-09-16T20:04:09Z

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java

@@ -62,7 +62,8 @@ public static boolean test_getCallerClass_Helper_Reflection_fromBootExtWithAnnot
 			cls = (Class<?>) method.invoke(null, new Object[0]);

 			boolean isClassNameExpected;
-			if (VersionCheck.major() >= 18) {
+			String useDMHStr = System.getProperties().getProperty("jdk.reflect.useDirectMethodHandle");
+			if (VersionCheck.major() >= 18 && (useDMHStr != null && useDMHStr.equals("true"))) {


Suggested change

if (VersionCheck.major() >= 18 && (useDMHStr != null && useDMHStr.equals("true"))) {

if (VersionCheck.major() >= 18 && Boolean.getBoolean("jdk.reflect.useDirectMethodHandle")) {

L65 can be removed.

Thanks a lot, I have applied the recommendation in https://github.com/eclipse-openj9/openj9/compare/7f66c8ff8a083767606602b2700d5fb56ad462d9..b58d7f658a0a54755bde06d23247ae4e544796f5 and also have verified the failing test.

JasonFengJ9

LGTM

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java

jcl/src/java.base/share/classes/java/lang/System.java

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java

keithc-ca · 2024-09-17T18:39:38Z

Please update the commit messages so no line in the body is longer than 72 characters. See https://github.com/eclipse/omr/blob/master/CONTRIBUTING.md#commit-guidelines for guidance.

In JEP 416, core reflection was re-implemented with Method Handles with the purpose of simplifying adding new language features. While analyzing performance of workload based on large enterprise based application, I observed that this was causing visible performance degradation. Disabling the use of direct method handles for reflection calls while we are working on improving the performance with direct method handles. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

Previously for JEP416, adjustments were made to ReflectionMHTests to adapt the frames for method handles. This test was failing when `-Djdk.reflect.useDirectMethodHandle=false` option is set. Adding an additional check in the test to see if the property is set to true to call for InjectedInvoker check. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

r30shah · 2024-09-17T18:45:49Z

Made sure the body width in https://github.com/eclipse-openj9/openj9/compare/25c26bfcc6f46952e991489d5cd72a6c8c0001c7..3fc532db53abed10c6097298e214d30ba7560ea5

keithc-ca · 2024-09-17T19:17:05Z

Jenkins test sanity,extended alinux jdk21,jdk23

r30shah · 2024-09-18T12:24:56Z

Looking into the failure in sanity.functional -

17:28:23  Test start time: 2024/09/17 21:28:23 Coordinated Universal Time
17:28:23  Running command: "/home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/jdkbinary/j2sdk-image/bin/java"  -Xcompressedrefs -Xjit -Xgcpolicy:gencon  -Xshareclasses:name=ShareClassesCMLTests,testDoublePageSize -cp /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/utils/utils.jar org.openj9.test.ivj.Hanoi 2
17:28:23  Time spent starting: 5 milliseconds
17:28:23  Time spent executing: 24 milliseconds
17:28:23  Test result: FAILED
17:28:23   [ERR] JVMSHRC072E Command-line option "testDoublePageSize" unrecognised
17:28:23   [ERR] JVMJ9VM015W Initialization error for library j9shr29(2): JVMJ9VM009E J9VMDllMain failed
17:28:23   [ERR] Error: Could not create the Java Virtual Machine.
17:28:23   [ERR] Error: A fatal exception has occurred. Program will exit.
17:28:23  >> Success condition was not found: [Output match: Puzzle solved!]
17:28:23  >> Failure condition was not found: [Output match: Unhandled Exception]
17:28:23  >> Failure condition was not found: [Output match: Exception:]
17:28:23  >> Failure condition was not found: [Output match: corrupt]
17:28:23  >> Failure condition was not found: [Output match: Processing dump event]
17:28:23  >> Failure condition was not found: [Output match: JVM requested Java dump]
17:28:23  >> Failure condition was not found: [Output match: JVM requested Snap dump]
17:28:23  
17:28:23  Testing: Test 71-b: Re-use the shared cache in readonly mode, which should detect the page difference
17:28:23  Test start time: 2024/09/17 21:28:23 Coordinated Universal Time
17:28:23  Running command: "/home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/jdkbinary/j2sdk-image/bin/java"  -Xcompressedrefs -Xjit -Xgcpolicy:gencon  -Xshareclasses:name=ShareClassesCMLTests,verbose,readonly -Xtrace:print={j9shr.2345} -cp /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/utils/utils.jar org.openj9.test.ivj.Hanoi 2
17:28:23  Time spent starting: 4 milliseconds
17:28:23  Time spent executing: 50 milliseconds
17:28:23  Test result: FAILED
17:28:23   [ERR] [-Xshareclasses persistent cache enabled]
17:28:23   [ERR] [-Xshareclasses verbose output enabled]
17:28:23   [ERR] JVMTRCE003E Error processing trace option, detail: Unable to set tracepoint 2345 in j9shr - tracepoint id out of range
17:28:23   [ERR] JVMTRCE011W Module not configured for trace: j9shr
17:28:23   [ERR] JVMSHRC226E Error opening shared class cache file
17:28:23   [ERR] JVMSHRC336E Port layer error code = -108
17:28:23   [ERR] JVMSHRC337E Platform error message: No such file or directory
17:28:23   [ERR] JVMSHRC840E Failed to start up the shared cache.
17:28:23   [ERR] JVMJ9VM015W Initialization error for library j9shr29(11): JVMJ9VM009E J9VMDllMain failed
17:28:23   [ERR] Error: Could not create the Java Virtual Machine.
17:28:23   [ERR] Error: A fatal exception has occurred. Program will exit.
17:28:23  >> Success condition was not found: [Output match: Puzzle solved!]
17:28:23  >> Required condition was not found: [Output match: j9shr.2345\s+ - Mismatch in layer 0 composite cache osPageSize value]
17:28:23  >> Required condition was not found: [Output match: JVMSHRC858I The page size .* of the operating system is different from the page size .* of the system on which layer 0 cache is created]
17:28:23  >> Required condition was not found: [Output match: JVMSHRC274I Memory page protection disabled for cache]
17:28:23  >> Failure condition was not found: [Output match: Unhandled Exception]
17:28:23  >> Failure condition was not found: [Output match: Exception:]
17:28:23  >> Failure condition was not found: [Output match: corrupt]
17:28:23  >> Failure condition was not found: [Output match: Processing dump event]
17:28:23  >> Failure condition was not found: [Output match: JVM requested Java dump]
17:28:23  >> Failure condition was not found: [Output match: JVM requested Snap dump]
17:28:23  
17:28:23  Testing: Test 71-c: Re-use the shared cache, which should detect the page difference
17:28:23  Test start time: 2024/09/17 21:28:23 Coordinated Universal Time
17:28:23  Running command: "/home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/jdkbinary/j2sdk-image/bin/java"  -Xcompressedrefs -Xjit -Xgcpolicy:gencon  -Xshareclasses:name=ShareClassesCMLTests,verbose -Xtrace:print={j9shr.2345} -cp /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal_testList_1/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/utils/utils.jar org.openj9.test.ivj.Hanoi 2
17:28:23  Time spent starting: 4 milliseconds
17:28:24  Time spent executing: 630 milliseconds
17:28:24  Test result: FAILED
17:28:24   [OUT] Beginning puzzle.  Solving for 2 disks.
17:28:24   [OUT] Moved disk 0 to 1
17:28:24   [OUT] Moved disk 0 to 2
17:28:24   [OUT] Moved disk 1 to 2
17:28:24   [OUT] Puzzle solved!
17:28:24   [ERR] [-Xshareclasses persistent cache enabled]
17:28:24   [ERR] [-Xshareclasses verbose output enabled]
17:28:24   [ERR] JVMTRCE003E Error processing trace option, detail: Unable to set tracepoint 2345 in j9shr - tracepoint id out of range
17:28:24   [ERR] JVMTRCE011W Module not configured for trace: j9shr
17:28:24   [ERR] JVMSHRC236I Created shared classes persistent cache ShareClassesCMLTests
17:28:24   [ERR] JVMSHRC246I Attached shared classes persistent cache ShareClassesCMLTests
17:28:24   [ERR] JVMSHRC765I Memory page protection on runtime data, string read-write data and partially filled pages is successfully enabled
17:28:24   [ERR] JVMSHRC168I Total shared class bytes read=186684. Total bytes stored=3827940
17:28:24   [ERR] JVMSHRC818I Total unstored bytes due to the setting of shared cache soft max is 0. Unstored AOT bytes due to the setting of -Xscmaxaot is 0. Unstored JIT bytes due to the setting of -Xscmaxjitdata is 0

I do not think the failure in the build has anything to do with changes in this PR.

pshipton · 2024-09-18T12:29:09Z

I do not think the failure in the build has anything to do with changes in this PR.

Yes, it's a mismatch between the tests and the JVM.

keithc-ca requested changes Sep 13, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

r30shah force-pushed the disableDMHPR branch from 3dc041d to 42fb9ab Compare September 13, 2024 13:26

r30shah force-pushed the disableDMHPR branch from 42fb9ab to f30df47 Compare September 13, 2024 16:33

r30shah changed the title ~~WIP: Disable use of method handle for core reflection in JDK21 and above~~ WIP: Disable use of method handle for core reflection in JDK21 Sep 13, 2024

r30shah changed the title ~~WIP: Disable use of method handle for core reflection in JDK21~~ Disable use of method handle for core reflection in JDK21 Sep 13, 2024

JasonFengJ9 reviewed Sep 16, 2024

View reviewed changes

r30shah force-pushed the disableDMHPR branch from 7f66c8f to b58d7f6 Compare September 16, 2024 20:18

JasonFengJ9 approved these changes Sep 16, 2024

View reviewed changes

keithc-ca reviewed Sep 16, 2024

View reviewed changes

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java Outdated Show resolved Hide resolved

r30shah force-pushed the disableDMHPR branch 2 times, most recently from 522e847 to 3ca1654 Compare September 16, 2024 20:53

keithc-ca reviewed Sep 16, 2024

View reviewed changes

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java Outdated Show resolved Hide resolved

r30shah force-pushed the disableDMHPR branch from 3ca1654 to 57a5ddf Compare September 16, 2024 21:52

keithc-ca requested changes Sep 17, 2024

View reviewed changes

jcl/src/java.base/share/classes/java/lang/System.java Outdated Show resolved Hide resolved

...tional/cmdline_options_testresources/src_90/com/ibm/j9/getcallerclass/ReflectionMHTests.java Outdated Show resolved Hide resolved

r30shah force-pushed the disableDMHPR branch from 57a5ddf to 25c26bf Compare September 17, 2024 14:10

r30shah added 2 commits September 17, 2024 14:41

r30shah force-pushed the disableDMHPR branch from 25c26bf to 3fc532d Compare September 17, 2024 18:43

keithc-ca approved these changes Sep 18, 2024

View reviewed changes

keithc-ca merged commit 17a3fe8 into eclipse-openj9:master Sep 18, 2024
7 of 9 checks passed

r30shah deleted the disableDMHPR branch September 19, 2024 16:23

This was referenced Sep 19, 2024

jdk21 OpenJDK reflection failures #20200

Closed

Revert changes that disables use of Method Handle for core reflection #20206

Merged

pshipton added the comp:vm label Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable use of method handle for core reflection in JDK21 #20159

Disable use of method handle for core reflection in JDK21 #20159

r30shah commented Sep 12, 2024

r30shah commented Sep 12, 2024

keithc-ca commented Sep 13, 2024 •

edited

Loading

r30shah commented Sep 13, 2024

keithc-ca commented Sep 13, 2024

keithc-ca commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 Sep 16, 2024

r30shah Sep 16, 2024 •

edited

Loading

JasonFengJ9 left a comment

keithc-ca commented Sep 17, 2024

r30shah commented Sep 17, 2024

keithc-ca commented Sep 17, 2024

r30shah commented Sep 18, 2024

pshipton commented Sep 18, 2024

	if (VersionCheck.major() >= 18 && (useDMHStr != null && useDMHStr.equals("true"))) {
	if (VersionCheck.major() >= 18 && Boolean.getBoolean("jdk.reflect.useDirectMethodHandle")) {

Disable use of method handle for core reflection in JDK21 #20159

Disable use of method handle for core reflection in JDK21 #20159

Conversation

r30shah commented Sep 12, 2024

r30shah commented Sep 12, 2024

keithc-ca commented Sep 13, 2024 • edited Loading

r30shah commented Sep 13, 2024

keithc-ca commented Sep 13, 2024

keithc-ca commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 commented Sep 16, 2024

r30shah commented Sep 16, 2024

JasonFengJ9 Sep 16, 2024

Choose a reason for hiding this comment

r30shah Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

JasonFengJ9 left a comment

Choose a reason for hiding this comment

keithc-ca commented Sep 17, 2024

r30shah commented Sep 17, 2024

keithc-ca commented Sep 17, 2024

r30shah commented Sep 18, 2024

pshipton commented Sep 18, 2024

keithc-ca commented Sep 13, 2024 •

edited

Loading

r30shah Sep 16, 2024 •

edited

Loading