-
Notifications
You must be signed in to change notification settings - Fork 694
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
Remove ArrayEncoding usage from Digest and delete ArrayEncoding. #1731
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1731 +/- ##
==========================================
- Coverage 96.00% 95.99% -0.02%
==========================================
Files 137 137
Lines 20749 20753 +4
Branches 226 226
==========================================
+ Hits 19920 19921 +1
- Misses 795 798 +3
Partials 34 34 ☔ View full report in Codecov by Sentry. |
b2fd38e
to
2c1bd86
Compare
I still need to benchmark this. Will add benchmarks for |
@joshlf This removes the last |
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.
Nice!
2c1bd86
to
8151b91
Compare
8151b91
to
7c3534a
Compare
This has been blocked on adding (back) the benchmarks for |
7c3534a
to
148206f
Compare
Note: I originally tried an alternative implementation using `flat_map` that ended up being materially slower. To fix that performance regression I had to make the following change: ``` let mut output = Output([0; MAX_OUTPUT_LEN]); output .0 - .iter_mut() - .zip(input.iter().copied().flat_map(|Wrapping(w)| f(w))) + .chunks_mut(N) + .zip(input.iter().copied().map(|Wrapping(w)| f(w))) .for_each(|(o, i)| { - *o = i; + o.copy_from_slice(&i); }); output } ``` I verified that this generates the same assembly code as the original code on x86-64 using Rust 1.74.0, except that there are two additional 128-bit moves in `sha256_formta_output` to zero out the latter half of `Output`, which was intended.
Using the benchmark added in PR #1809, I found that my original implementation here did have a notable performance regression. To fix it, I had to make this change: let mut output = Output([0; MAX_OUTPUT_LEN]);
output
.0
- .iter_mut()
- .zip(input.iter().copied().flat_map(|Wrapping(w)| f(w)))
+ .chunks_mut(N)
+ .zip(input.iter().copied().map(|Wrapping(w)| f(w)))
.for_each(|(o, i)| {
- *o = i;
+ o.copy_from_slice(&i);
});
output
} I verified that this generates the same assembly code as the original code on x86-64 using Rust 1.74.0, except that there are two additional 128-bit moves in |
148206f
to
149904c
Compare
The codecov "project" coverage % "failure" is expected since this reduces the amount of covered code. |
No description provided.