Recognize Unsafe.copyMemory0 in JDK11 #10366

r30shah · 2020-08-11T13:04:26Z

OpenJ9 contains the optimization that recognizes the JNI call sun/misc/Unsafe.copyMemory and transforms it to System.arrayCopy calls, which is inlined and optimized on most of the platform. Due to implementation changes in JDK11 we can not apply this optimization to sun/misc/Unsafe.copyMemory method which in Java 11 acts as a java wrapper that calls the implementation of copyMemory method from jdk/internal/misc/Unsafe class. This implementation contains additional changes to check range for the source and destination (in case of illegal access throws RuntimeException) before calling the
actual JNI method jdk/internal/misc/Unsafe.copyMemory0 which can be transformed to System.arrayCopy call. Due to this behavioural changes, when compiling method that calls Unsafe.copyMemory, we need to make sure that optimizer is exposed to JNI call Unsafe.copyMemory0 and that call is recognized same way as JNI call sun/misc/Unsafe.copyMemory is recognized in Java 8 to optimize it further.

Signed-off-by: Rahil Shah rahil@ca.ibm.com

r30shah · 2020-08-11T13:05:59Z

@andrewcraik Can I please get this one reviewed, I am currently performing the sanity tests on the changes so marking it WIP.

r30shah · 2020-08-11T13:08:29Z

runtime/compiler/p/codegen/J9TreeEvaluator.cpp

@@ -11993,8 +11993,11 @@ J9::Power::CodeGenerator::inlineDirectCall(TR::Node *node, TR::Register *&result
            }
         break;
         }
-


I have a question to both @gita-omr and @andrewcraik , I see that P and x86 codegen further forces the Unsafe.copyMemory calls to be inlined in case optimizer misses it. I do not see we do it on Z. Is it possible (Apart from running with noOpt) that optimizer would miss this call to transform?

the inlining in the inliner is always optional - the inliner may run out of budget before considering a call or may choose to spend budget elsewhere. While things like always inline bypass most of the heuristics related to budget - the current inliner is eager and so once it has consumed its budget it will stop considering callsites which can leave 'alwaysInline' methods not inlined.

IMO we should move that reduction out of the codegen evaluators and right into the J9RecognizedCallTransformer so all codegens can transparently take advantage. I'd try to avoid duplicating the already duplicated logic on Z.

andrewcraik

Overall I do not like the Java version checks - we have avoided those to make it easier to test the JIT since all code runs all the time. I would rather we look at reworking this to better match how other multi-implementation methods have been handled with less macros.

runtime/compiler/codegen/J9RecognizedMethodsEnum.hpp

andrewcraik · 2020-08-11T13:31:54Z

runtime/compiler/il/J9Node.cpp

@@ -299,7 +299,12 @@ J9::Node::processJNICall(TR::TreeTop * callNodeTreeTop, TR::ResolvedMethodSymbol
 #endif

   if (comp->canTransformUnsafeCopyToArrayCopy() &&
-       (methodSymbol->getRecognizedMethod() == TR::sun_misc_Unsafe_copyMemory))
+#if JAVA_SPEC_VERSION == 8


I really don't think the method specific naming convention here makes sense. I think we should have an Unsafe_copyMemory_intrinsic which is what we match the sun/misc/Unsafe.copyMemory to under JDK8 and what we match the copyMemory0 to under JDK11. I think also that if there is a method to be inlined like jdk_internal_misc_Unsafe_copyMmeory0 that recognition can be matched unconditionally - it will only match when we are running JDK11 onward when the method moved. If there is a windows where the method moved but was native we can just check for the method being native before we recognize it and make all that matching code uncondition.

Using the Unsafe_copyMemory_intrinsic that is what OMR can reference and avoid Java specific issues to surface at that level of the architecture.

@andrewcraik I have made changes that removes the #if macros that was checking the JAVA Version. I agree that we do not have to guard jdk_internal_misc_Unsafe_copyMmeory0 with JAVA SPEC version. Right now in JDK8 and JDK11, we have seen JNI methods sun/misc/Unsafe.copyMemory and jdk/internal/misc/Unsafe.copyMemory0 respectively and I have added an additional check to make sure it does the transformation only when it is JNI method in d1952e6

r30shah · 2020-08-12T00:40:44Z

runtime/compiler/il/J9Node.cpp

@@ -2212,6 +2212,23 @@ J9::Node::setDontInlinePutOrderedCall()
   }

 bool
+J9::Node::isUnsafeCopyMemoryIntrinsic()


I found that having a query on the call node is the most convenient place as we can have all the information to check whether call node is JNI call and it is one of the recognized copyMemory method.

r30shah · 2020-08-12T00:42:32Z

@andrewcraik My functional testing has finished and I have also tried to address the comments. This one is good for review. Also this change is dependant on OMR counterpart eclipse/omr#5464

andrewcraik · 2020-08-12T18:27:49Z

is this change dependent on the omr? We do want to remove the OMR function but can do so once it has been added in the OpenJ9 layer - there shouldn't be a conflict.

Looks like there was some kind of functional problem in the Travis build:

=== Output from failing command(s) repeated here ===
* For target jdk__packages_attribute.done:
Exception in thread "main" java/lang/StackOverflowError
	at jdk/internal/misc/Unsafe.copyMemory (java.base@9/Unsafe.java:1293)

@r30shah Can you investigate please?

r30shah · 2020-08-12T20:43:35Z

@andrewcraik We can not let this guy go first before the OMR one is merged. The failure happening in the travis build is because of this https://github.com/eclipse/openj9/blob/7ff0542881fc7b7b3e82f96b7358c10729c9fb53/runtime/compiler/il/J9Node.cpp#L385-L415
Changes in this PR will skip wrapping up objects for JNI call jdk/internal/misc/Unsafe.copyMemory0 and that seems like causing the failure. Changes in eclipse/omr#5464 modifies the call to transformUnsafeCopyMemoryCall to return to false. So it won't break the build. May cause regression due to not doing transformation temporarily till this guy gets in. I am marking this WIP and removing WIP from the OMR

andrewcraik · 2020-08-17T13:56:27Z

Jenkins test sanity all jdk15 depends eclipse/omr#5464

r30shah · 2020-08-17T14:42:47Z

Failure on Linux on Z seems like because of missing commits from OMR (eclipse/omr@7970637 , eclipse/omr@c84f6f9)
I will rebase the OMR once the build on other platforms gets finished,

andrewcraik · 2020-08-17T17:11:15Z

@r30shah interesting 390 failure:

10:19:12  Assertion failed at /home/jenkins/workspace/Build_JDK15_s390x_linux_Personal/build/linux-s390x-server-release/vm/compiler/../omr/compiler/z/env/OMRCPU.cpp:117: self()->is_at_least_test(p)
10:19:12  VMState: 0x0005ff04
10:19:12  	processor Unknown(1) failed, _supportedArch z15(9), _processorDescription.processor z15(10)
10:19:12  compiling java/lang/System.getEncoding(I)Ljava/lang/String; at level: cold

doesn't look like your change, but something you or @fjeremic might be interested in.

r30shah · 2020-08-17T17:44:54Z

@andrewcraik , Yes failures are not related to this changes, either I need to rebase the repo or something with the z15 machine on which this build was ran (https://ci.eclipse.org/openj9/computer/rh7-390-2/)
I am waiting for the last couple of ongoing builds finished before I re base both repo.
@fjeremic Do you think it is rebase issue or something more we need to check with the machine?

runtime/compiler/optimizer/J9ValuePropagation.cpp

r30shah · 2020-08-17T20:00:44Z

I have rebased the changes with latest master on both OpenJ9 and OMR, if changes are OK, can we get the builds on s390 linux?

runtime/compiler/il/J9Node.cpp

runtime/compiler/il/J9Node.hpp

fjeremic · 2020-08-17T21:27:46Z

runtime/compiler/p/codegen/J9TreeEvaluator.cpp

@@ -11993,8 +11993,11 @@ J9::Power::CodeGenerator::inlineDirectCall(TR::Node *node, TR::Register *&result
            }
         break;
         }
-


IMO we should move that reduction out of the codegen evaluators and right into the J9RecognizedCallTransformer so all codegens can transparently take advantage. I'd try to avoid duplicating the already duplicated logic on Z.

fjeremic · 2020-08-18T18:14:35Z

runtime/compiler/il/J9Node.hpp

+   /**
+    * Checks  and return true if the callNode is JNI Call to Unsafe.copyMemory
+    */
+   bool isUnsafeCopyMemoryIntrinsic();


This API seems very very specific for it to exist in the Node class. I worry this class will turn into a kitchen sink eventually. Does this really need to exist? It seems it gets used in two locations:

In processJNICall which we can just replace with:

if (comp->canTransformUnsafeCopyToArrayCopy() && (methodSymbol->getRecognizedMethod() == TR::sun_misc_Unsafe_copyMemory || methodSymbol->getRecognizedMethod() == TR::jdk_internal_misc_Unsafe_copyMemory0))

In transformUnsafeCopyMemoryCall which we can just inline a partial version of this API.

Are we sure introducing this extra complexity into the Node class outweighs the benefits?

@fjeremic Reason I chose to introduce this API in Node class is that it contains all the information that it needed to verify if the JNI call can be transformed. Idea behind using this intrinsic is that if in later version of JDK if there are changes in the call chain, we only need to make changes in one place.
There is another potential use of this API in recognized call transformer to identify and transform this call as you suggested in #10366 (comment) , which I think we should pursue and remove the complexity and duplicate code from the codegen.

That sounds reasonable. Thanks for explaining further uses.

r30shah · 2020-08-27T15:01:06Z

@andrewcraik Yes, WIP is for OMR dependency.

andrewcraik · 2020-08-28T13:20:25Z

Jenkins test sanity all jdk15 depends eclipse/omr#5464

andrewcraik · 2020-09-02T20:10:41Z

Jenkins test sanity all jdk15 depends eclipse/omr#5464

andrewcraik · 2020-09-03T20:47:10Z

Jenkins test sanity all jdk15 depends eclipse/omr#5464

r30shah · 2020-09-03T21:21:10Z

Seems like some this one also needs a rebase, @andrewcraik Do you want me to wait for rest of the tests to finish and then rebase or rebase it and you can launch the test again?

andrewcraik · 2020-09-08T14:57:53Z

@r30shah if you can rebase I'll just rerun the testing - I want to be sure things are good before we start the merge sequence since there will be a perf regression if we don't merge this one quickly enough

r30shah · 2020-09-08T15:09:55Z

@andrewcraik I agree, rebased the changes on both OMR and OpenJ9 repo.

andrewcraik · 2020-09-08T15:12:49Z

Jenkins test sanity all jdk15 depends eclipse/omr#5464

r30shah · 2020-09-08T16:10:06Z

Seems like couple of infrastructure failures in the test.

11:19:58  stderr: fatal: unable to access 'https://github.com/eclipse/openj9.git/': Failed to connect to github.com port 443: Connection timed out
11:19:58  
11:19:58  	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2436)
11:19:58  	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:2050)
11:19:58  	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$500(CliGitAPIImpl.java:84)
11:19:58  	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:572)
11:19:58  	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$2.execute(CliGitAPIImpl.java:801)
11:19:58  	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:161)
11:19:58  	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:154)
11:19:58  	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
11:19:58  	at hudson.remoting.UserRequest.perform(UserRequest.java:54)
11:19:58  	at hudson.remoting.Request$2.run(Request.java:369)
11:19:58  	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
11:19:58  	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
11:19:58  	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
11:19:58  	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
11:19:58  	at java.lang.Thread.run(Thread.java:748)

andrewcraik · 2020-09-09T13:56:15Z

Jenkins test sanity xlinux,xlinuxxl,zlinuxxl,plinux jdk15

r30shah · 2020-09-09T17:32:49Z

@andrewcraik In the last launch build (#10366 (comment)) , OMR counterpart was not mentioned (eclipse/omr#5464) , hence we are seeing the stackoverflow errors.

andrewcraik · 2020-09-09T17:35:20Z

sorry my bad - let's try again

andrewcraik · 2020-09-09T17:36:01Z

Jenkins test sanity xlinux,xlinuxxl,zlinuxxl,plinux jdk15 depends eclipse/omr#5464

andrewcraik · 2020-09-14T17:52:00Z

Jenkins test sanity all jdk15

andrewcraik · 2020-09-14T17:52:14Z

Jenkins test sanity xlinux,xlinuxxl,zlinuxxl,plinux jdk15 depends eclipse/omr#5464

r30shah · 2020-09-14T20:56:54Z

Not sure what is happening, but failing builds have not picked up the eclipse/omr#5464. In the passed builds, I do not see on console log it explicitly checks out the changes from the OMR Counterpart.

andrewcraik · 2020-09-14T21:07:07Z

Jenkins test sanity xlinux,xlinuxxl,zlinuxxl,plinux jdk15 depends eclipse/omr#5464

r30shah · 2020-09-15T13:19:22Z

@andrewcraik I see all the platform on which this tests were launched in #10366 (comment) are passed. Not sure why I am seeing mac and windows. But this one is good to merge IMO. If you are OK with it, we need to first let OMR changes in then OpenJ9.

r30shah · 2020-09-16T12:38:20Z

Change in eclipse/omr#5464 has been merged yesterday and passed OMR acceptance, this one is good to go, removing WIP

OpenJ9 contains the optimization that recognizes the JNI call sun/misc/Unsafe.copyMemory and transforms it to System.arrayCopy calls, which is inlined and optimized on most of the platform. Due to implementation changes in JDK11 we can not apply this optimization to sun/misc/Unsafe.copyMemory method which in Java 11 acts as a java wrapper that calls the implementation of copyMemory method from jdk/internal/misc/Unsafe class. This implementation contains additional changes to check range for the source and destination (in case of illegal access throws RuntimeException) before calling the actual JNI method jdk/internal/misc/Unsafe.copyMemory0 which can be transformed to System.arrayCopy call. Due to this behavioural changes, when compiling method that calls Unsafe.copyMemory, we need to make sure that optimizer is exposed to JNI call Unsafe.copyMemory0 and that call is recognized same way as JNI call sun/misc/Unsafe.copyMemory is recognized in Java 8 to optimize it further. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

Transformation from Unsafe.copyMemory to System.arrayCopy was Java specific transformation. This commit moves the transformation to OpenJ9. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

andrewcraik · 2020-09-16T13:08:31Z

Previously passed all sanity and we need to get this in now to avoid seeing a perf regression so merging.

r30shah commented Aug 11, 2020

View reviewed changes

r30shah mentioned this pull request Aug 11, 2020

WIP: Recognize Unsafe.copyMemory0 in JDK11 eclipse/omr#5460

Closed

andrewcraik suggested changes Aug 11, 2020

View reviewed changes

andrewcraik added comp:jit perf labels Aug 11, 2020

andrewcraik self-assigned this Aug 11, 2020

r30shah mentioned this pull request Aug 11, 2020

Remove Unsafe.copyMemory transformation from OMR eclipse/omr#5464

Merged

r30shah force-pushed the unsafePR branch 2 times, most recently from 5bba1b1 to cdb436c Compare August 12, 2020 00:31

r30shah changed the title ~~WIP: Recognize Unsafe.copyMemory0 in JDK11~~ Recognize Unsafe.copyMemory0 in JDK11 Aug 12, 2020

r30shah commented Aug 12, 2020

View reviewed changes

r30shah changed the title ~~Recognize Unsafe.copyMemory0 in JDK11~~ WIP: Recognize Unsafe.copyMemory0 in JDK11 Aug 12, 2020

fjeremic reviewed Aug 17, 2020

View reviewed changes

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved

r30shah force-pushed the unsafePR branch 3 times, most recently from ff0b1e1 to 5b7e35a Compare August 17, 2020 19:58

fjeremic reviewed Aug 17, 2020

View reviewed changes

r30shah force-pushed the unsafePR branch 2 times, most recently from 91cf3d3 to fa538f2 Compare August 18, 2020 01:02

fjeremic reviewed Aug 18, 2020

View reviewed changes

r30shah force-pushed the unsafePR branch from fa538f2 to 82e89e9 Compare August 20, 2020 14:57

r30shah force-pushed the unsafePR branch from 9939a4f to dc19287 Compare September 1, 2020 19:23

r30shah force-pushed the unsafePR branch from dc19287 to 46b265e Compare September 8, 2020 15:08

r30shah force-pushed the unsafePR branch from 46b265e to 49d0470 Compare September 14, 2020 14:24

r30shah changed the title ~~WIP: Recognize Unsafe.copyMemory0 in JDK11~~ Recognize Unsafe.copyMemory0 in JDK11 Sep 16, 2020

r30shah added 2 commits September 16, 2020 08:38

Move transformUnsafeCopyMemoryCall in OpenJ9

c8663fc

Transformation from Unsafe.copyMemory to System.arrayCopy was Java specific transformation. This commit moves the transformation to OpenJ9. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

r30shah force-pushed the unsafePR branch from 49d0470 to c8663fc Compare September 16, 2020 12:38

andrewcraik merged commit b27e6ee into eclipse-openj9:master Sep 16, 2020

pshipton mentioned this pull request Sep 17, 2020

JDK11 Linux ppc64le java/util/Hashtable/SerializationDeadlock.java *** stack smashing detected *** #10630

Closed

BradleyWood mentioned this pull request Aug 25, 2021

Liberty app server does not start without value propagation #13314

Closed

Recognize Unsafe.copyMemory0 in JDK11 #10366

Recognize Unsafe.copyMemory0 in JDK11 #10366

Conversation

r30shah commented Aug 11, 2020

r30shah commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewcraik 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

r30shah commented Aug 12, 2020

andrewcraik commented Aug 12, 2020

r30shah commented Aug 12, 2020

andrewcraik commented Aug 17, 2020

r30shah commented Aug 17, 2020

andrewcraik commented Aug 17, 2020

r30shah commented Aug 17, 2020

r30shah commented Aug 17, 2020

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

r30shah commented Aug 27, 2020

andrewcraik commented Aug 28, 2020

andrewcraik commented Sep 2, 2020

andrewcraik commented Sep 3, 2020

r30shah commented Sep 3, 2020

andrewcraik commented Sep 8, 2020

r30shah commented Sep 8, 2020

andrewcraik commented Sep 8, 2020

r30shah commented Sep 8, 2020

andrewcraik commented Sep 9, 2020

r30shah commented Sep 9, 2020

andrewcraik commented Sep 9, 2020

andrewcraik commented Sep 9, 2020

andrewcraik commented Sep 14, 2020

andrewcraik commented Sep 14, 2020

r30shah commented Sep 14, 2020

andrewcraik commented Sep 14, 2020

r30shah commented Sep 15, 2020

r30shah commented Sep 16, 2020

andrewcraik commented Sep 16, 2020