-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Z: Accelerate ArraysSupport.vectorizedHashCode #19455
Z: Accelerate ArraysSupport.vectorizedHashCode #19455
Conversation
Internal Jenkins sanity test: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/21953/ The only failure I see is a JITServer/CRIU failure that look to be unrelated |
@r30shah fyi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add some brief information about the changes in the commit message i.e, recognize and accelerate array hashcode generation using SIMD instruction?
@@ -97,6 +97,7 @@ J9::Z::CodeGenerator::initialize() | |||
!TR::Compiler->om.canGenerateArraylets()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename the JIT option TR_DisableSIMDStringHashCode
as it is now used for vectorizedHashCode as well. I think it makes sense to rename to TR_DisableSIMDHashCode
.
And in place where we are recognizing the method in code-gen, we can use the environment flag to disable the String or Array hashcode acceleration (Before suppressing the inlining through traditional inliner).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also rename the code generator flag SupportsInlineStringHashCode
to SupportsInlineHashCode
and use that instead of adding a SupportsInlineVectorizedHashCode
flag?
The TR_DisableSIMDStringHashCode
option is defined in OMR, it might make more sense to migrate it to just be in OpenJ9, and rename it as part of that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the advantage over having that option in OMR was that it can be applied to selective method, this may not be something we would want when we move that to OpenJ9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes, overall looks good to me. Once you make those cosmetic changes, will launch builds
Adds ArraysSupport.vectorizedHashCode as a recognized method and accelerate with a SIMD instruction sequence. The hash function is the same as String.hashCode, so the instruction generation logic is commoned between the two into a helper function. Signed-off-by: Spencer Comin <spencer.comin@ibm.com>
d126425
to
5613c65
Compare
Here's a couple instruction selection logs: Integer array
Byte array
|
jenkins test sanity zlinux jdk17,jdk21 |
@Spencer-Comin Given that this is accelerating the Arrays.hashcode implementation in JDK21 and onwards - I think it would be good idea to evaluate the performance through a benchmark (JMH based / bumblebench) to see how much speed up we got |
Here's some benchmarking results for
We are getting a 3-4x improvement, which makes sense given that we go from consuming 1 element at a time in the scalar path to 4 at a time in the vector path. |
jenkins test sanity zlinux jdk17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I see JDK17 failed with some infra related issue so launched the test again,
Is the Instruction selection have any dependency condition being generated ? If yes I do not see that in the snippet you pasted.
Yes, I forgot to include the dependency conditions from the log. Here's the integer array snippet again with the dependency conditions:
|
Hi @Spencer-Comin All the tests have been passed, so going to merge this one. One odd think I am seeing with the generated instructions is the Load and LGFR instruction. |
ArraysSupport.vectorizedHashCode
[1] is an intrinsic candidate introduced in JDK21. The hash function used is the same as the one used for hashing Strings, so this change leverages that by recognizingArraysSupport.vectorizedHashCode
and using the preexisting accelerated implementation ofString.hashCode
.[1] https://github.com/ibmruntimes/openj9-openjdk-jdk21/blob/openj9/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java#L200