Accelerate Unsafe CAS Intrinsics on Power and X #19991
Conversation
|
@hzongaro Is there someone who can help take a look at the X and Common changes? In particular that I am updating @zl-wang Could you take a look at the Power codegen changes to make sure there are no problems? |
|
are we supposed to go around the "public" (not sure about this) Unsafe interface/class and support the internal Unsafe APIs? i.e., do typical developers import jdk.internal.misc.Unsafe? In addition to recognize the former class' APIs, we need to recognize the internal newer ones as well? @tajila @vijaysun-omr what is your take? |
|
Are you suggesting that there are "public" equivalents of the apis being discussed in this PR (that we already accelerate or plan to accelerate) ? If not, then internal (non-public) or not, it could have a bearing on performance, right ? |
|
there are two Unsafe classes in JDK ... for example jdk21: APIs in the former class basically use the latter's for implementation. Something like below: if programmers are expected (if they ever do) to use the former only, it might not be necessary to go behind it and deal with the new names. |
|
Only these ones exist in These ones require going through |
Most new applications and the JDK itself have switched to using jdk.internal.misc.Unsafe. sun.misc.Unsafe is still available but will likely go at some point. One thing to remember is that in JDK8 there is only sun.misc.Unsafe. |
Thanks for the clarification. |
| { | ||
| case 4: | ||
| oldValue = oldVNode->getInt(); | ||
| if (oldValue >= LOWER_IMMED && oldValue <= UPPER_IMMED) |
There was a problem hiding this comment.
this "if" block is expected to be common to all cases (even for future byte/short CAS support). why not move/common it to after-if?
There was a problem hiding this comment.
I can move it outside the switch block.
|
|
||
| case TR::jdk_internal_misc_Unsafe_compareAndExchangeInt: | ||
| // As above, we only want to inline the JNI methods, so add an explicit test for isNative() | ||
| if (!methodSymbol->isNative()) |
There was a problem hiding this comment.
is this check only worth of debugging value? i.e. we explicitly don't want to inline/intercept the CAS call for debugging reason (?).
There was a problem hiding this comment.
The original problem traces back to this commit: 111f0b0
Going from Java 8 to 11, compareAndSwap[Object|Int|Long] were changed to being wrappers to call Native versions with the same name. So there is both a java and native that matches the same recognized method.
Going from 11 to 17, compareAndSetObject and compareAndExchangeObject were changed from native to Java wrapper that calls the new names compareAndSetReference and compareAndExchangeReference that do the same thing.
That commit I linked to deals with compareAndSwap[Object|Int|Long] and compareAndSetObject. Acceleration for compareAndExchangeObject didn't exist so that case didn't need to be handled.
Based on the comment in the commit, the change explicitly went in to deal with incorrect behavior. But, it doesn't say what it is and I'm not exactly sure what the problem is either.
I copied the same check for the acceleration of compareAndExchange[Reference|Int|Long]. But, those are always native in every JDK level they exist in. So I can remove the check for those cases.
compareAndSet[Reference|Int|Long] is also always native but uses the same recognized methods as compareAndSwap[Object|Int|Long] so I can't remove the check for that case without updating Z and arm code which is currently future work.
There was a problem hiding this comment.
I talked to @JamesKingdon and he pointed me towards this issue from around 7 years ago: #321
From the issue the test would hang. The exact reasoning is not listed but somehow it was determined that preventing the non-native wrapped calls of the CAS methods from being turned into accelerated assembly made the problem stop. As a conservative fix, the optimization was limited to just the native methods and to leave it to the inliner to inline the wrapper methods.
There was a problem hiding this comment.
The linked issue was create due to an external test that I do not have access to. The issue also includes a simple program which could reproduce the issue but it no longer runs. The is mention in the issue of a dependency on a server that I assume is no longer running. So it does not seem to be possible to reproduce the problem.
I suspect that the original fix was only hiding an issue because it doesn't make much sense that it would have actually fixed the problem. Putting down accelerated assembly should have worked when replacing non-native Unsafe CAS call wrappers. As it would turn out, while creating this PR I encountered an issue with reference counting inside the X fast path generation for Unsafe CAS. While a different path is taken today, 7 years ago compareAndSwapObject, which appeared to cause the problem, went through inlineCompareAndSwapNative and that's where I found the problem with reference counting. It is also possible the problem was something else fixed in the last 7 years. Without being able to run the test, I can't confirm that.
|
|
||
| if ((node->isUnsafeGetPutCASCallOnNonArray() || !TR::Compiler->om.canGenerateArraylets()) && node->isSafeForCGToFastPathUnsafeCall()) | ||
| { | ||
| static bool disableCAEIntrinsic = feGetEnv("TR_DisableCAEIntrinsic") != NULL; |
There was a problem hiding this comment.
why not common and move this feGetEnv outside switch statement?
| resultReg = VMinlineCompareAndSetOrExchange(node, cg, 8, true); | ||
| return true; | ||
| } | ||
| else if ((node->isUnsafeGetPutCASCallOnNonArray() || !TR::Compiler->om.canGenerateArraylets()) && node->isSafeForCGToFastPathUnsafeCall()) |
There was a problem hiding this comment.
i saw great performance value in doing fast-path CAS long in 32bit mode, given the fact that we started to only support 64bit hardware a few years ago.
There was a problem hiding this comment.
compareAndExchangeLong was first added in JDK11. I believe OpenJ9 does not support 32bit JDK11 (and later) builds? If not, the 32bit acceleration path for compareAndExchangeLong will never be used even if added in.
|
I added a few more fixes mostly related to problems on x that showed up after more rigorous testing. I missed adding Setting Setting I found another location I missed adding I got one of the native checks backwards inside |
|
I discovered a problem on x where the accelerated version of I eventually tracked down the problem to this line: openj9/runtime/compiler/x/codegen/J9TreeEvaluator.cpp Line 10529 in 91b5703
To differentiate Unsafe CAS calls from the other cases, there is a check for the node being an icall since the other cases are not icalls. This check is no longer valid with the introduction of support for The commit I just added changes the check to a generic call check. There is also a check for the call being to a recognized method. This check now checks for specific recognized methods which are: With this new change, the performance has returned to the expected area. |
|
With the latest change, all the tests that I wrote to try out different edges cases are working as expected. @hzongaro, I know it has only been a few days since my last x related change but have you had a chance to continue your code review? |
I hadn't had a chance to come back to it. I'll take another look at it now. |
IBMJimmyk
force-pushed
the
UnsafeCAS
branch
from
1c9ba0d
to
6f77021
Compare
|
I have some more tests that look like they are failing now. So I'll need to figure out the problem before this can be merged. |
|
I found the problem.
So NULL is passed in as the This is the temporary platform check that is here until support is added for every platform. It assumed I will double check these platform checks do not cause any other problems. |
IBMJimmyk
force-pushed
the
UnsafeCAS
branch
from
6f77021
to
39d626b
Compare
|
I retested after I added the fix to @hzongaro Can you look over the latest changes? If they are good I think this is finally ready to be merged in. |
| if (nullptr == c) | ||
| { | ||
| c = TR::comp(); | ||
| } |
There was a problem hiding this comment.
It just seems bizarre to me that isUnsafeCAS had a default value of NULL for c, and it never even used the argument. Looking at the histories of isUnsafeCAS and isUnsafeWithObjectArg, it looks like they've always been that way.
I don't want to delay this change any further, so may I suggest that you open follow up pull requests for OpenJ9 and OMR to change the prototype declarations for isUnsafeCAS and isUnsafeWithObjectArg to either remove the TR::Compilation parameter or to make the parameter no longer optional?
There was a problem hiding this comment.
Yes. I can do that as well.
|
May I ask you to remove "WIP:" from the titles of this pull request and eclipse/omr#7438? |
|
Sorry for the late comment. May I ask you to reformat your commit comment so that lines do not exceed 72 characters in length, per https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md#commit-guidelines? There is also a typo in the commit comment: "differtiate" -> "differentiate" |
|
Jenkins test sanity,sanity.openjdk all jdk8,jdk11,jdk17,jdk21 |
|
Sorry - the testing I initiated will fail because I forgot to include the |
IBMJimmyk
changed the title
Adds support for the following recognized methods: CompareAndExchangeObject //JDK11 CompareAndExchangeReference //JDK17,21 CompareAndExchangeInt //JDK11,17,21 CompareAndExchangeLong //JDK11,17,21 Similar to their CompareAndSet counterparts, the JIT acceleration does not support CAE on static fields so the createUnsafeCASCallDiamond function was updated to also work on the CAE methods. The accelerated CAE code was built on top of the existing accelerated CAS support on both Power and X. Removed recognized CAS enums from isUnsafePut. They are not Unsafe put methods and these cases could not be triggered. Even before my changes, setting TR_UseOldCompareAndSwapObject and TR_DisableCASInlining would cause a crash on x. It doesn't really make sense to set both at the same time but I fixed it anyways since it was quick. VMwrtbarWithoutStoreEvaluator is used for several different opcdes include arraycopy, ArrayStoreCHK, writeBarrier and Unsafe CAS calls. This is only used for the Unsafe CAS calls that store an object. To differtiate Unsafe CAS calls from the other cases, there is a check for the node being an icall. This check is no longer valid with the introduction of support for compareAndExchangeReference. The node in this case is an acall. This changes the check to a generic call check. There is also a check for the call being to a recognized method. This check now checks for specific recognized methods which are: sun_misc_Unsafe_compareAndSwapObject_jlObjectJjlObjectjlObject_Z jdk_internal_misc_Unsafe_compareAndExchangeObject jdk_internal_misc_Unsafe_compareAndExchangeReference Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
IBMJimmyk
force-pushed
the
UnsafeCAS
branch
from
39d626b
to
fda8a6f
Compare
|
I fixed the commit message. |
|
Jenkins test sanity,sanity.openjdk all jdk8,jdk11,jdk17,jdk21 depends eclipse/omr#7438 |
|
The Windows failure looks to be this: I don't think that is related to my change. |
I will rerun testing for the infrastructure-related failures. @IBMJimmyk, may I ask you to take a closer look at the JDK 8 sanity.functional failures for Windows to verify that they are unrelated to your changes? |
|
Jenkins test sanity,sanity.openjdk windows jdk21 depends eclipse/omr#7438 |
|
I'll take a look at the sanity.functional failures for Windows. |
|
Jenkins test sanity,sanity.openjdk win jdk21 depends eclipse/omr#7438 |
|
Jenkins test sanity.functional xmac jdk11,jdk21 depends eclipse/omr#7438 |
|
Jenkins test sanity.openjdk aix jdk17 depends eclipse/omr#7438 |
1 similar comment
|
Jenkins test sanity.openjdk aix jdk17 depends eclipse/omr#7438 |
|
All of the JDK8 Windows failures fall into the same pattern. The test workload itself is successful and prints the expected result for the test and shortly afterwards there is a segmentation fault in Here is an example from The test successfully prints out I have tried to reproduce the problem but have not been successful. When the tests were run as a part of this PR, this error occurred about 12 times total. I tried rerunning the tests 5 times each with a build with and without my changes and they all passed. I stumbled upon this similar looking problem: In that issue, there is also a test with a successful result followed by a crash in I think by builds go up to here in OpenJ9: |
|
I was able to reproduce the JDK8 Windows failures on a build that excluded both my Unsafe CAS work and the @hzongaro Are there any other concerns that need to be addressed? |
|
Failures are due to known issues. Performing a coordinated merge with eclipse/omr#7438 |
Adds support for the following recognized methods:
Similar to their CompareAndSet counterparts, the JIT acceleration does not support CAE on static fields so the createUnsafeCASCallDiamond function was updated to also work on the CAE methods.
The accelerated CAE code was built on top of the existing accelerated CAS support on both Power and X.
Edit:
X changes are dependent on this OMR PR:
eclipse/omr#7438
Behavior of
inlineCompareAndSwapNativeandinlineCompareAndSwapObjectNativeinJ9TreeEvaluator.cppmust match behavior inwillBeEvaluatedAsCallByCodeGeninOMRCodeGenerator.cpp