Updated recognized methods for newer JDKs #18383

IBMJimmyk · 2023-11-01T14:17:02Z

Method recognition has been updated for the following three cases:

As of JDK12, ConcurrentHashMap.tabAt now calls Unsafe.getReferenceAcquire instead of Unsafe.getObjectVolatile. Unsafe.getReferenceAcquire is now recognized so it triggers the Unsafe Fast Path opt.

As of JDK12, ConcurrentHashMap.setTabAt now calls Unsafe.putReferenceRelease instead of Unsafe.putObjectVolatile. Unsafe.putReferenceRelease is also now recognized for Unsafe Fast Path.

As of JDK15, the signature for HashMap.getNode no longer has an int parameter. An entry for this case has been added so the method is forced to be inlined like it was before.

IBMJimmyk · 2023-11-01T14:18:55Z

@vijaysun-omr @zl-wang Could you take a look at this?

vijaysun-omr · 2023-11-01T16:32:33Z

I assume the presence of two signatures that could be recognized for getNode is to account for different language versions, before and after JDK15. I think it would be good to double check that this does not cause any issues in releases prior to JDK15 with the recognition.

I would also appreciate a quick review from @hzongaro for this PR.

zl-wang · 2023-11-01T17:33:58Z

runtime/compiler/env/j9method.cpp

@@ -2887,6 +2888,7 @@ void TR_ResolvedJ9Method::construct()
      {x(TR::sun_misc_Unsafe_putFloatVolatile_jlObjectJF_V,         "putFloatRelease",   "(Ljava/lang/Object;JF)V")},
      {x(TR::sun_misc_Unsafe_putDoubleVolatile_jlObjectJD_V,        "putDoubleRelease",  "(Ljava/lang/Object;JD)V")},
      {x(TR::sun_misc_Unsafe_putObjectVolatile_jlObjectJjlObject_V, "putObjectRelease",  "(Ljava/lang/Object;JLjava/lang/Object;)V")},
+      {x(TR::sun_misc_Unsafe_putObjectVolatile_jlObjectJjlObject_V, "putReferenceRelease",  "(Ljava/lang/Object;JLjava/lang/Object;)V")},



what is the motivation to keep the enum name of TR::sun_misc_Unsafe_putObjectVolatile_jlObjectJjlObject_V for method putReferenceRelease?

I was following the convention that was already being used in the area. For example one line up the existing putObjectRelease also reuses TR::sun_misc_Unsafe_putObjectVolatile_jlObjectJjlObject_V.

putObjectRelease does nothing but call putObjectVolatile.
putReferenceRelease does nothing but call putReferenceVolatile.

putObjectRelease, putObjectVolatile and putReferenceVolatile are all recognized as TR::sun_misc_Unsafe_putObjectVolatile_jlObjectJjlObject_V. So it seemed to me that putReferenceRelease should also do the same.

ok. for these particular methods, this might be the right way to go. otherwise, more works are required to optimize them, since their bytecode(s) contain another call.

zl-wang · 2023-11-01T17:34:51Z

runtime/compiler/env/j9method.cpp

@@ -2921,6 +2923,7 @@ void TR_ResolvedJ9Method::construct()
      {x(TR::sun_misc_Unsafe_getFloatVolatile_jlObjectJ_F,          "getFloatAcquire",   "(Ljava/lang/Object;J)F")},
      {x(TR::sun_misc_Unsafe_getDoubleVolatile_jlObjectJ_D,         "getDoubleAcquire",  "(Ljava/lang/Object;J)D")},
      {x(TR::sun_misc_Unsafe_getObjectVolatile_jlObjectJ_jlObject,  "getObjectAcquire",  "(Ljava/lang/Object;J)Ljava/lang/Object;")},
+      {x(TR::sun_misc_Unsafe_getObjectVolatile_jlObjectJ_jlObject,  "getReferenceAcquire",  "(Ljava/lang/Object;J)Ljava/lang/Object;")},


IBMJimmyk · 2023-11-01T17:43:46Z

The new recognized HashMap.getNode without the int parameter does not match anything from JDK8-14. Those JDK versions only have the older 2 parameter version of HashMap.getNode that includes the int parameter.

zl-wang · 2023-11-01T17:47:03Z

runtime/compiler/env/j9method.cpp

@@ -2141,6 +2141,7 @@ void TR_ResolvedJ9Method::construct()
      {x(TR::java_util_HashMap_findNullKeyEntry,      "findNullKeyEntry",           "()Ljava/util/HashMap$Entry;")},
      {x(TR::java_util_HashMap_get,                   "get",           "(Ljava/lang/Object;)Ljava/lang/Object;")},
      {x(TR::java_util_HashMap_getNode,               "getNode",       "(ILjava/lang/Object;)Ljava/util/HashMap$Node;")},
+      {x(TR::java_util_HashMap_getNode,               "getNode",       "(Ljava/lang/Object;)Ljava/util/HashMap$Node;")},
      {x(TR::java_util_HashMap_putImpl,               "putImpl",       "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;")},


as well, same enum name for essentially different recognized methods, any implications? hoped these different methods don't exist at the same time in a single level of language.

That was the same question I had, maybe I just asked it in an unclear manner .

For this one, I reused it because it was almost the same method and all the locations that used the recognized method wanted to do the exact same thing. But, they aren't completely identical so it might be better to separate them even if the two versions don't ever exist in the same JDK level at the moment.

Yes that would be cleaner. Thanks.

I have now split off the new Hashmap.get to map to java_util_HashMap_getNode_Object.

Thanks for making that change. I think people have often reused enumerated values for recognized methods because it was convenient thing to do. However, it's a potential source of future bugs if someone makes a change to the treatment of a recognized method without realizing that more than one method might be affected.

zl-wang

LGTM

hzongaro

Over all, I think the changes look good. Just one suggestion for some pre-existing code in UnsafeFastPath.cpp.

Also, the commit guidlines ask that the first line of a commit be in the imperative mood. May I ask you to change "Updated recognized methods for newer JDKs" to something like "Update recognized methods for newer JDKs", and if you are not going to squash the second commit, change "New recognized method for HashMap.getNode" to something like "Add new recognized method for HashMap.getNode"?

hzongaro · 2023-11-02T14:08:32Z

runtime/compiler/env/j9method.cpp

@@ -2141,6 +2141,7 @@ void TR_ResolvedJ9Method::construct()
      {x(TR::java_util_HashMap_findNullKeyEntry,      "findNullKeyEntry",           "()Ljava/util/HashMap$Entry;")},
      {x(TR::java_util_HashMap_get,                   "get",           "(Ljava/lang/Object;)Ljava/lang/Object;")},
      {x(TR::java_util_HashMap_getNode,               "getNode",       "(ILjava/lang/Object;)Ljava/util/HashMap$Node;")},
+      {x(TR::java_util_HashMap_getNode,               "getNode",       "(Ljava/lang/Object;)Ljava/util/HashMap$Node;")},
      {x(TR::java_util_HashMap_putImpl,               "putImpl",       "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;")},


Thanks for making that change. I think people have often reused enumerated values for recognized methods because it was convenient thing to do. However, it's a potential source of future bugs if someone makes a change to the treatment of a recognized method without realizing that more than one method might be affected.

hzongaro · 2023-11-02T14:13:25Z

runtime/compiler/optimizer/UnsafeFastPath.cpp

-                  break;
+                  case TR::java_util_concurrent_ConcurrentHashMap_tabAt:
+                  case TR::java_util_concurrent_ConcurrentHashMap_setTabAt:
+                     isArrayOperation = true;


As long as you're modifying this file, may I ask you to add a break at the end of this case? There doesn't seem to be any reason for it to want to fall through, and if someone adds another case to this switch in the future, they might not notice they'll need to add a break.

I can do this. The commit title will also be updated and the commits squashed.

Method recognition has been updated for the following three cases: As of JDK12, ConcurrentHashMap.tabAt now calls Unsafe.getReferenceAcquire instead of Unsafe.getObjectVolatile. Unsafe.getReferenceAcquire is now recognized so it triggers the Unsafe Fast Path opt. As of JDK12, ConcurrentHashMap.setTabAt now calls Unsafe.putReferenceRelease instead of Unsafe.putObjectVolatile. Unsafe.putReferenceRelease is also now recognized for Unsafe Fast Path. As of JDK15, the signature for HashMap.getNode no longer has an int parameter. An entry for this case has been added so the method is forced to be inlined like it was before. All locations that use the existing HashMap.getNode recognized method entry have been updated to also work with the new one. This change also includes minor code clean up inside UnsafeFastPath.cpp. A break has been added to a switch block for code clarity and some whitespace has been fixed. Signed-off-by: jimmyk <jimmyk@ca.ibm.com> added break

IBMJimmyk · 2023-11-02T15:07:09Z

I added the break statement (and some more whitespace fixes), updated the commit title and squashed the commits. I think this addresses all comment so far.

hzongaro

Looks good. Thanks!

zl-wang · 2023-11-02T17:30:47Z

jenkins test sanity plinux,aix,xlinux,zlinuxjit jdk8,jdk17

r30shah · 2023-11-03T15:47:52Z

Jimmy reached out regarding to failure seen on Z with CRIU tests in the build. The failure seen in the test are seen in nightly builds on OpenJ9 jenkins as well [1][2]. Searching around, Jason has already opened up issue on Jenkins two days ago reporting same failure in internal jenkins (#18384). Will look into those failures. As far as this PR goes, I can confirm, failures seen on Z are not related to the changes in this PR.

[1]. https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_s390x_linux_Nightly_testList_0/591/tapResults/
[2]. https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_s390x_linux_Nightly_testList_0/591/tapResults/

IBMJimmyk · 2023-11-03T17:54:39Z

https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_ppc64le_linux_Personal/350/
Failing tests are a bunch of cmdLineTester_criu_* tests. Error messages are similar to:

 [OUT] CRIU needs to have the CAP_SYS_ADMIN or the CAP_CHECKPOINT_RESTORE capability: 
 [OUT] setcap cap_checkpoint_restore+eip criu
 [OUT] Exception in thread "main" org.eclipse.openj9.criu.SystemCheckpointException: Could not dump the JVM processes, err=-52
 [OUT] 	at openj9.criu/org.eclipse.openj9.criu.CRIUSupport.checkpointJVMImpl(Native Method)
 [OUT] 	at openj9.criu/org.eclipse.openj9.criu.CRIUSupport.checkpointJVM(CRIUSupport.java:811)
 [OUT] 	at org.openj9.criu.CRIUTestUtils.checkPointJVM(CRIUTestUtils.java:77)
 [OUT] 	at org.openj9.criu.CRIUTestUtils.checkPointJVM(CRIUTestUtils.java:65)
 [OUT] 	at org.openj9.criu.CRIUTestUtils.checkPointJVM(CRIUTestUtils.java:61)
 [OUT] 	at org.openj9.criu.CRIUSimpleTest.checkpoints(CRIUSimpleTest.java:59)
 [OUT] 	at org.openj9.criu.CRIUSimpleTest.main(CRIUSimpleTest.java:38)

This is a known issue and exists in builds that do not include my recognized method changes.

https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_x86-64_linux_Personal/454/
The one failing test is cmdLineTester_jvmtitests_hcr_6. Error looks like this:

Testing: rc018
Test start time: 2023/11/02 19:54:23 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk17_j9_sanity.functional_x86-64_linux_Personal_testList_0/jdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xnocompressedrefs  -Xdump    -agentlib:jvmtitest=test:rc018 -cp "/home/jenkins/workspace/Test_openjdk17_j9_sanity.functional_x86-64_linux_Personal_testList_0/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar:/home/jenkins/workspace/Test_openjdk17_j9_sanity.functional_x86-64_linux_Personal_testList_0/aqa-tests/TKG/../TKG/lib/asm-all.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 3 milliseconds
***[TEST INFO 2023/11/02 19:58:23] ProcessKiller detected a timeout after 240000 milliseconds!***

I am currently looking into why there was a timeout.

https://openj9-jenkins.osuosl.org/job/Test_openjdk8_j9_sanity.functional_s390x_linux_jit_Personal/4/
Failing test here is testJitserverArguments_0 and fails during the "Test SSL success condition" section. I am also currently looking into this. Although, I have doubts this is related to my change since the newly recognized methods do not exist under JDK8.

IBMJimmyk · 2023-11-06T14:22:35Z

I was able to reproduce both the timeout issue in test rc018 in cmdLineTester_jvmtitests_hcr_6 and the "Test SSL success condition" failure in testJitserverArguments_0 in builds that do not include my recognized method changes.

So all of the failures in testing look to be existing issues that are unrelated to my changes.

zl-wang · 2023-11-06T14:26:41Z

good. merging ...

zl-wang reviewed Nov 1, 2023

View reviewed changes

zl-wang self-assigned this Nov 2, 2023

zl-wang approved these changes Nov 2, 2023

View reviewed changes

hzongaro reviewed Nov 2, 2023

View reviewed changes

vijaysun-omr approved these changes Nov 2, 2023

View reviewed changes

IBMJimmyk force-pushed the UnsafeFastPathJDK17 branch from 74e65bb to 0d0bd0f Compare November 2, 2023 15:06

hzongaro approved these changes Nov 2, 2023

View reviewed changes

zl-wang merged commit 0d0f5ed into eclipse-openj9:master Nov 6, 2023
14 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated recognized methods for newer JDKs #18383

Updated recognized methods for newer JDKs #18383

IBMJimmyk commented Nov 1, 2023

IBMJimmyk commented Nov 1, 2023

vijaysun-omr commented Nov 1, 2023

zl-wang Nov 1, 2023

IBMJimmyk Nov 1, 2023

zl-wang Nov 1, 2023

zl-wang Nov 1, 2023

IBMJimmyk commented Nov 1, 2023

zl-wang Nov 1, 2023

vijaysun-omr Nov 1, 2023

IBMJimmyk Nov 1, 2023

vijaysun-omr Nov 1, 2023

IBMJimmyk Nov 2, 2023

hzongaro Nov 2, 2023

zl-wang left a comment

hzongaro left a comment

hzongaro Nov 2, 2023

hzongaro Nov 2, 2023

IBMJimmyk Nov 2, 2023

IBMJimmyk commented Nov 2, 2023

hzongaro left a comment

zl-wang commented Nov 2, 2023

r30shah commented Nov 3, 2023

IBMJimmyk commented Nov 3, 2023

IBMJimmyk commented Nov 6, 2023

zl-wang commented Nov 6, 2023

Updated recognized methods for newer JDKs #18383

Updated recognized methods for newer JDKs #18383

Conversation

IBMJimmyk commented Nov 1, 2023

IBMJimmyk commented Nov 1, 2023

vijaysun-omr commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IBMJimmyk commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zl-wang left a comment

Choose a reason for hiding this comment

hzongaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IBMJimmyk commented Nov 2, 2023

hzongaro left a comment

Choose a reason for hiding this comment

zl-wang commented Nov 2, 2023

r30shah commented Nov 3, 2023

IBMJimmyk commented Nov 3, 2023

IBMJimmyk commented Nov 6, 2023

zl-wang commented Nov 6, 2023