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

Use sha2 to calculate SHA256 #11795

Merged
merged 1 commit into from Mar 5, 2023
Merged

Use sha2 to calculate SHA256 #11795

merged 1 commit into from Mar 5, 2023

Conversation

ahollmann
Copy link

crypto-hash seems to be unmaintained (last real commit on Feb 27, 2020) and forces duplicate crates due to outdated dependencies.

Minimal change to replace crypto-hash by sha2.

This was already attempted in #4545 (back in 2017).

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2023
@weihanglo weihanglo mentioned this pull request Mar 2, 2023
@ehuss
Copy link
Contributor

ehuss commented Mar 3, 2023

Thanks! Have you done any performance profiling? An example might be vendoring something large like the rust repo.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

You may also need to bump the version of cargo-util in crates/cargo-util/Cargo.toml. "0.2.4" is a good candidate as there seems no SemVer breaking change.

@ahollmann
Copy link
Author

ahollmann commented Mar 3, 2023

Results for /usr/bin/time --verbose cargo vendor

I can do more in depth performance profiling later.

        Command being timed: "/home/aeh/git/cargo/target/release/cargo vendor"
        User time (seconds): 1.40
        System time (seconds): 2.28
        Percent of CPU this job got: 97%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:03.77
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 64116
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 8279
        Voluntary context switches: 2657
        Involuntary context switches: 158
        Swaps: 0
        File system inputs: 128
        File system outputs: 2286976
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0
        Command being timed: "cargo vendor"
        User time (seconds): 1.35
        System time (seconds): 2.44
        Percent of CPU this job got: 97%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:03.91
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 58688
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 8
        Minor (reclaiming a frame) page faults: 11106
        Voluntary context switches: 2862
        Involuntary context switches: 259
        Swaps: 0
        File system inputs: 5744
        File system outputs: 2304416
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

hyperfine output for cargo using cargo crypto-hash vs. cargo using sha2

hyperfine --warmup 1 --export-markdown cargo-profile.md '~/cargo-profiling/cargo-crypto-hash vendor' '~/cargo-profiling/cargo-sha2 vendor' 
Benchmark 1: ~/cargo-profiling/cargo-crypto-hash vendor
  Time (mean ± σ):      4.814 s ±  0.096 s    [User: 1.752 s, System: 2.981 s]
  Range (min … max):    4.626 s …  5.005 s    10 runs
 
Benchmark 2: ~/cargo-profiling/cargo-sha2 vendor
  Time (mean ± σ):      4.759 s ±  0.057 s    [User: 1.711 s, System: 2.967 s]
  Range (min … max):    4.698 s …  4.852 s    10 runs
 
Summary
  '~/cargo-profiling/cargo-sha2 vendor' ran
    1.01 ± 0.02 times faster than '~/cargo-profiling/cargo-crypto-hash vendor'

Hotspot profile with perf record/report : Time taken in cargo_util::sha256::Sha256::update

perf record --call-graph dwarf ~/cargo-profiling/cargo-crypto-hash-debug vendor

  • 10.74% 0.00% cargo-crypto-ha cargo-crypto-hash-debug [.] cargo_util::sha256::Sha256::update

perf record --call-graph dwarf ~/cargo-profiling/cargo-sha2-debug vendor

  • 11.33% 0.01% cargo-sha2-debu cargo-sha2-debug [.] cargo_util::sha256::Sha256::update

@ahollmann
Copy link
Author

Performance seems to be nearly the same at least on my system (Linux x86_64).

I can provide more performance data if you need it.

@@ -39,7 +39,7 @@ impl Sha256 {

pub fn finish(&mut self) -> [u8; 32] {
let mut ret = [0u8; 32];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole function can be simplified to avoid the copy:

self.0.finalize_reset().into()

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Done.

@arlosi
Copy link
Contributor

arlosi commented Mar 4, 2023

Thanks! Perf is not impacted and Cargo already depends on sha2.

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 4, 2023

📌 Commit 2f0ecf5 has been approved by arlosi

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2023
@bors
Copy link
Collaborator

bors commented Mar 4, 2023

⌛ Testing commit 2f0ecf5 with merge 137c2e1...

@bors
Copy link
Collaborator

bors commented Mar 5, 2023

☀️ Test successful - checks-actions
Approved by: arlosi
Pushing 137c2e1 to master...

@bors bors merged commit 137c2e1 into rust-lang:master Mar 5, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Mar 7, 2023
23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d
2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000

- Add `CARGO_PKG_README` (rust-lang/cargo#11645)
- path dependency: fix cargo-util version (rust-lang/cargo#11807)
- Adding display of which target failed to compile (rust-lang/cargo#11636)
- Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790)
- Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805)
- Enhance the doc of timing report with graphs (rust-lang/cargo#11798)
- Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791)
- Use sha2 to calculate SHA256 (rust-lang/cargo#11795)
- gitoxide progress bar fixes (rust-lang/cargo#11800)
- Check publish_to_alt_registry publish content (rust-lang/cargo#11799)
- chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797)
- chore: Update base64 (rust-lang/cargo#11796)
- Fix some doc typos (rust-lang/cargo#11794)
- chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761)
- Some cleanup for unstable docs (rust-lang/cargo#11793)
- gitoxide integration: fetch (rust-lang/cargo#11448)
- patch can conflict on not activated packages (rust-lang/cargo#11770)
- fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630)
- Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783)
- feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688)
- feat: Use test name for dir when running tests (rust-lang/cargo#11738)
- Jobserver cleanup (rust-lang/cargo#11764)
- Fix help string for  "--charset" option of "cargo tree" (rust-lang/cargo#11785)

Note that some 3rd-party licensing allowed list changed due to the
introducion of `gix` dependency
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2023
Update cargo

25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d
2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000

- Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812)
- Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810)
- Add `CARGO_PKG_README` (rust-lang/cargo#11645)
- path dependency: fix cargo-util version (rust-lang/cargo#11807)
- Adding display of which target failed to compile (rust-lang/cargo#11636)
- Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790)
- Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805)
- Enhance the doc of timing report with graphs (rust-lang/cargo#11798)
- Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791)
- Use sha2 to calculate SHA256 (rust-lang/cargo#11795)
- gitoxide progress bar fixes (rust-lang/cargo#11800)
- Check publish_to_alt_registry publish content (rust-lang/cargo#11799)
- chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797)
- chore: Update base64 (rust-lang/cargo#11796)
- Fix some doc typos (rust-lang/cargo#11794)
- chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761)
- Some cleanup for unstable docs (rust-lang/cargo#11793)
- gitoxide integration: fetch (rust-lang/cargo#11448)
- patch can conflict on not activated packages (rust-lang/cargo#11770)
- fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630)
- Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783)
- feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688)
- feat: Use test name for dir when running tests (rust-lang/cargo#11738)
- Jobserver cleanup (rust-lang/cargo#11764)
- Fix help string for  "--charset" option of "cargo tree" (rust-lang/cargo#11785)

---

~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess?

r? `@ehuss`
cc `@Muscraft`
@ehuss ehuss added this to the 1.70.0 milestone Mar 9, 2023
saethlin pushed a commit to saethlin/miri that referenced this pull request Mar 11, 2023
Update cargo

25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d
2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000

- Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812)
- Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810)
- Add `CARGO_PKG_README` (rust-lang/cargo#11645)
- path dependency: fix cargo-util version (rust-lang/cargo#11807)
- Adding display of which target failed to compile (rust-lang/cargo#11636)
- Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790)
- Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805)
- Enhance the doc of timing report with graphs (rust-lang/cargo#11798)
- Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791)
- Use sha2 to calculate SHA256 (rust-lang/cargo#11795)
- gitoxide progress bar fixes (rust-lang/cargo#11800)
- Check publish_to_alt_registry publish content (rust-lang/cargo#11799)
- chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797)
- chore: Update base64 (rust-lang/cargo#11796)
- Fix some doc typos (rust-lang/cargo#11794)
- chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761)
- Some cleanup for unstable docs (rust-lang/cargo#11793)
- gitoxide integration: fetch (rust-lang/cargo#11448)
- patch can conflict on not activated packages (rust-lang/cargo#11770)
- fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630)
- Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783)
- feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688)
- feat: Use test name for dir when running tests (rust-lang/cargo#11738)
- Jobserver cleanup (rust-lang/cargo#11764)
- Fix help string for  "--charset" option of "cargo tree" (rust-lang/cargo#11785)

---

~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess?

r? `@ehuss`
cc `@Muscraft`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants