Skip to content
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

Enable SHA512 acceleration on Aarch4 Android, Linux, and Apple targets. #1978

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

briansmith
Copy link
Owner

No description provided.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 94.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.28%. Comparing base (c160d98) to head (49add64).

Files Patch % Lines
src/polyfill/ptr.rs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1978      +/-   ##
==========================================
- Coverage   96.29%   96.28%   -0.01%     
==========================================
  Files         136      137       +1     
  Lines       20677    20723      +46     
  Branches      226      226              
==========================================
+ Hits        19910    19954      +44     
- Misses        732      735       +3     
+ Partials       35       34       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith briansmith force-pushed the b/sha512-apple-2 branch 11 times, most recently from cc782fe to 87b7687 Compare March 2, 2024 00:37
@briansmith briansmith marked this pull request as ready for review March 2, 2024 00:38
@briansmith
Copy link
Owner Author

@BusyJay I added aarch64-apple-darwin code coverage measurement to CI in PR #1980 and #1979, and then rebased this on top of that. That way we can measure the code coverage of your changes (as amended by me) accurately.

@briansmith briansmith force-pushed the b/sha512-apple-2 branch 3 times, most recently from 75d071b to 41651bc Compare March 2, 2024 01:06
}
debug_assert_eq!(len, mem::size_of_val(&value));
if len != mem::size_of_val(&value) {
return false;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...why is codecov telling me that this line is being hit?

let rc = unsafe { libc::sysctlbyname(name, value_ptr, &mut len, core::ptr::null_mut(), 0) };
// All the conditions are separated so we can observe them in code coverage.
if rc != 0 {
return false;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why codecov is saying this is being hit.

@briansmith briansmith force-pushed the b/sha512-apple-2 branch 2 times, most recently from 1bb7778 to 5cd744a Compare March 2, 2024 03:28
fn getauxval(type_: c_ulong) -> c_ulong;
cfg_if::cfg_if! {
if #[cfg(target_arch = "aarch64")] {
use libc::{getauxval,AT_HWCAP, HWCAP_AES, HWCAP_PMULL, HWCAP_SHA2, HWCAP_SHA512};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BusyJay When I rebased your change on top of PR #1982, this was the only non-trivial merge conflict. Instead of using your definition of HWCAP_SHA512, I used the one from libc.

BusyJay and others added 5 commits March 1, 2024 21:30
Tested on M1 and time consumed by sha512 is reduced to half.
Make it easy to test the dynamic detection for other aarch64-apple-*
targets.
Use the `libc` crate so we don't have to maintain the binding for
`sysctlbyname`, and so `libc` will take care of the linking to
libc as needed..

Wrap `sysctlbyname` in a safe wrapper. Separate the branches for
the `sysctlbyname` results so we can see them individually in code
coverage.
@briansmith briansmith merged commit 5fb6541 into main Mar 4, 2024
143 of 145 checks passed
@briansmith briansmith deleted the b/sha512-apple-2 branch March 4, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants