Skip to content

Switch from SHA-1 to SHA-512 - #109

Merged
alexcrichton merged 1 commit into
mozilla:masterfrom
alexcrichton:sha512
May 15, 2017
Merged

Switch from SHA-1 to SHA-512#109
alexcrichton merged 1 commit into
mozilla:masterfrom
alexcrichton:sha512

Conversation

@alexcrichton

Copy link
Copy Markdown
Contributor

Local benchmarking showed that the implementation of SHA-512 in the ring crate
is 3x faster than the implementation of SHA-1 in the sha1 crate. I've also
noticed that 80%+ of sccache's runtime on a fully cached build is spent hashing.
With this change I noticed a decrease from 108s to 92s when building a fully
cached LLVM from the network. Not a huge win but if the network were faster
could perhaps add up!

Closes #108

Comment thread src/util.rs Outdated
Digest { inner: Context::new(&SHA512) }
}

/// Calculate the SHA-1 digest of the contents of `path`, running

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It should read SHA-512 here.

Comment thread src/compiler/rust.rs Outdated
@@ -842,7 +840,6 @@ bar.rs:
drop(env_logger::init());
let f = TestFixture::new();
// SHA-1 digest of an empty file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment forgotten to be removed.

@alexcrichton

Copy link
Copy Markdown
Contributor Author

Ah just realized I should update the ring dependency, so this now depends on ring 0.9

@alexcrichton

Copy link
Copy Markdown
Contributor Author

Ok I've rebased over #112, but something I realized was that ring 0.9 depends on Rust 1.17 so I bumped that dep, if that's a problem though I can change it back.

@est31

est31 commented May 13, 2017

Copy link
Copy Markdown

@alexcrichton if you bump the required rust version in the .travis.yml, you should also bump it in the README.

@luser

luser commented May 15, 2017

Copy link
Copy Markdown
Contributor

Ok I've rebased over #112, but something I realized was that ring 0.9 depends on Rust 1.17 so I bumped that dep, if that's a problem though I can change it back.

I don't love constantly bumping our version requirement to chase latest stable, but as long as it's mentioned in the README and enforced in our CI config it's not a dealbreaker for anything.

@luser luser left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good! Sorry I put sha1 in so many names so you had to change them all...

Comment thread src/compiler/rust.rs Outdated
// 5. The sha-1 digests of all files listed on the commandline (self.externs)
args.hash(&mut HashToDigest { digest: &mut m });
// 4. The digest-1 digests of all source files (this includes src file from cmdline).
// 5. The digest-1 digests of all files listed on the commandline (self.externs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two comments got search-and-replaced into nonsense. :)

Comment thread src/compiler/rust.rs
&pool).wait().unwrap();
let mut m = sha1::Sha1::new();
let m = Digest::new();
let empty_digest = m.finish();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good call!

Local benchmarking showed that the implementation of SHA-512 in the *ring* crate
is 3x faster than the implementation of SHA-1 in the `sha1` crate. I've also
noticed that 80%+ of sccache's runtime on a fully cached build is spent hashing.
With this change I noticed a decrease from 108s to 92s when building a fully
cached LLVM from the network. Not a huge win but if the network were faster
could perhaps add up!

Closes mozilla#108
@alexcrichton
alexcrichton merged commit 1b8c364 into mozilla:master May 15, 2017
@alexcrichton
alexcrichton deleted the sha512 branch May 15, 2017 20:51
@alexcrichton

alexcrichton commented May 15, 2017

Copy link
Copy Markdown
Contributor Author

@luser as a heads up I updated the "install rust" code in AppVeyor to use rustup, I think the old manifests may have not made their way to 1.17.0

@luser

luser commented May 16, 2017

Copy link
Copy Markdown
Contributor

Thanks! I copied that script from somewhere and haven't looked at it in a while. :)

Xanewok pushed a commit to Xanewok/sccache that referenced this pull request Dec 8, 2021
tottoto pushed a commit to tottoto/sccache that referenced this pull request Feb 6, 2026
Both tutorials were missing .await on the client connect call, causing the compiler to issue errors.
Thanks for alce#6260 on the #tonic-users discord channel for providing the answer.
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.

3 participants