Allow inlining of java/lang/ref/Reference.refersTo method #13619
Conversation
There was a problem hiding this comment.
LGTM. I would like @0xdaryl or someone else from the opt team to review the new code from TR_J9VM::inlineNativeCall(), though.
Also, I know that the existing code tests if (comp->getGetImplInlineable()), but is that really necessary? Can't we query fe->getGetImplInlineable() directly and eliminate those accesors from the comp object?
| @@ -6715,6 +6715,12 @@ TR_J9VM::classHasBeenReplaced(TR_OpaqueClassBlock * clazzPointer) | |||
|
|
|||
| bool | |||
| TR_J9VMBase::isGetImplInliningSupported() | |||
There was a problem hiding this comment.
After delivering this PR, maybe we can change OMRCompilation.cpp to not use
self()->setGetImplInlineable(self()->fej9()->isGetImplInliningSupported()); anymore, so that we can eliminate TR_J9VMBase::isGetImplInliningSupported() and rely only on TR_J9VMBase::isGetImplAndRefersToInliningSupported()
There was a problem hiding this comment.
Thanks, Marius @mpirvu. Yes - I have another pair of pull requests that I'll open to do that. One to change OMRCompilation.cpp, and another to remove the getImpl-specific methods from OpenJ9.
|
I'm just in the process of doing one last set of performance runs. Assuming that completes successfully, I'll move this PR from "draft" to "ready for review" status. |
|
This fix appears to resolve the performance problem identified in issue #13596, so I'm marking it as ready for review. The original benchmark I was analyzing still shows a performance regression when run with a JDK 17 build of OpenJ9 versus a JDK 11 build. I'll open a separate issue for that, once I've identified the reason. |
|
Daryl @0xdaryl, may I ask you to review, or can you suggest a reviewer? |
|
Sorry for the churn. I've moved this back to draft status as I've seen an unexpected test failure. |
hzongaro
force-pushed
the
inline-reference-refersto
branch
from
c5c014b
to
399b27f
Compare
The refersTo method was introduced to java.lang.ref.Reference in Java 16. The JIT compiler is able to inline the method under the same conditions as it is able to inline Reference.getImpl. This change introduces new query methods and fields to check and record whether both methods can be inlined. The existing VM::isGetImplInliningSupported and Compilation::setGetImplInlineable methods are invoked by OMR, so they will remain in OpenJ9 until a subsequent pull request against OMR replaces calls to those now obsolete methods. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
A Reference.refersTo(Object) method, introduced in Java 16, tests whether the referent is the same object as the parameter. Some places in the JCL that used to perform comparisons of the referent using Reference.get now use Reference.refersTo. That affects performance of such uses as Reference.get was implemented using a call to Reference.getImpl which the JIT compiler recognized and inlined if possible. This change adds recognition of Reference.refersTo to the JIT, which will now inline a call to it under the same conditions as it would inline a call to Reference.getImpl. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
If TR_J9VM::inlineNativeCall fails to inline a method, it sometimes returns NULL and sometimes returns the original call node. If it returns NULL and the call node is a JNI call, the caller considers whether any special processing of the call node is required by calling TR::Node::processJNICall. In the case of Reference.refersTo, the call is not inlined if the Metronome GC policy is in effect, but inlineNativeCall was returning the original call node. That signalled to the caller that no special consideration of whether the call node was a JNI call was needed, but it was in fact needed. Fixed this by having TR_J9VM::inlineNativeCall return NULL if it is unable to inline Reference.refersTo. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
hzongaro
force-pushed
the
inline-reference-refersto
branch
from
399b27f
to
97af55c
Compare
|
Eighteen months later. . . . I mentioned in an earlier comment that I had seen unexpected failures with my fix - those failures were seen with the Metronome gcpolicy. I spent some time last week investigating those failures, and discovered that it had to do with how Here are performance results using the microbenchmark I mentioned in a comment in issue 13596 running with and without these changes in JDK 8 and JDK 17 - three runs of each. Higher numbers for "score" are better: JDK 8 without the fix
JDK 17 without the fix
JDK 8 with the fix
JDK 17 with the fix
|
|
Jenkins test sanity all jdk8,jdk17 |
|
Jenkins test sanity aix jdk17 Previous failure is infrastructural. |
|
CRIU test failure is infrastructural. I highly doubt that test has anything to do with the code in this PR. |
The
Reference.refersTo(Object)method, introduced in Java 16, tests whether the referent is the same object as the parameter. Some places in the JCL that used to perform comparisons of the referent using a call toReference.getnow useReference.refersTo. That affects performance of such uses asReference.getwas implemented using a call toReference.getImplwhich the JIT compiler recognized and inlined if possible.This change adds recognition of
Reference.refersToto the JIT, which will now inline a call to it under the same conditions as it would inline a call toReference.getImpl.In addition, there are several methods in the JIT that are used to record or test whether
Reference.getImplcan be inlined. This change introduces duplicate methods that are used for the bothgetImplandrefersTo. References to the old methods that mentiongetImplalone still exist in OMR and will be removed after this pull request is merged.Resolves issue #13596