-
Notifications
You must be signed in to change notification settings - Fork 10
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
Calculate CRC32C using SIMD acceleration #66
Conversation
Signed-off-by: Li Zhanhui <[email protected]>
@lizhanhui Thank you very much for putting this together 👍 The crc32c crates does not seem to provide hardware acceleration for ARM. I just want to make sure this change will not degrade performance for environment where hardware acceleration is not leveraged. |
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.
Thank you very much for adding a unit test to show consistency and for adding a benchmark!
LGTM.
Good catch! 🙌 |
Not yet, let's add one comparison between crc32c baseline and crc. |
Hardware acceleration for ARM is guarded by nightly https://github.com/zowens/crc32c/blob/d70d4e253c4570a073f485d62bc30c4fd64a4b1f/src/lib.rs#L51 |
Signed-off-by: lizhanhui <[email protected]>
After forking crate crc32c and introducing features to enable/disable hardware acceleration, I got the following result on a 2015-early Intel Macbook Air. (Note the previous run is performed on a M1 Macbook Pro, AArch64, nightly Rust toolchain used) without hardware acceleration Conclusion: crc32c is 4X faster than crc using pure software algorithm with hardware acceleration Conclusion: crc32c is about 10X faster with hardware acceleration @timvisee @agourlay My Mac at home is kind of old. You can try it on your modern one. |
As discussed in the issue, there is still some optimization space ahead. Some 5X should be possible. |
@lizhanhui Excellent work, you convinced me that the change is a net positive for all users. Please remove the last commit to get back to a published version and I am good for ✔️ |
…ion" This reverts commit c7a5b24.
Thanks for the compliment. The last commit has been reverted. |
@agourlay Hold on. I think I spot an issue of crate |
CRC with custom seed/initial value is somewhat unclear in RFCs. Here is a relevant discussion Crate 'crc' takes refin = true into account while 'crc32c' does not. I created a pull request to make them behave consistently. In addition, I added an extra test case to ensure we will get exactly same checksum values. Eventually, this pull request is completely sound and ready to be merged. |
…value Signed-off-by: lizhanhui <[email protected]>
Signed-off-by: lizhanhui <[email protected]>
@lizhanhui Very nice work! Thank you so much for your efforts. Am I correct that we're currently just waiting on zowens/crc32c#50, after which this can be merged? |
No, we can merge directly. This pull request does not use the API that is inconsistent with crate 'crc' anymore. |
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.
👍
This pull request contains two sections: one benchmark and modification to replace crate
crc
with crc32c.A typical micro-bench run shows the following metrics