[golang-dev] Re: code review 6820096: crypto/sha1: Make sha-1 do block mixup in place (issue 6820096)

$ cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 44
model name : Intel(R) Xeon(R) CPU W3690 @ 3.47GHz
...



https://codereview.appspot.com/6820096/

Looks pretty good to me as well. I'll run some benchmarks on 32 bit
platforms once mine get through building the overnight changes.


https://codereview.appspot.com/6820096/diff/4002/src/pkg/crypto/sha1/sha1block.go
File src/pkg/crypto/sha1/sha1block.go (right):

https://codereview.appspot.com/6820096/diff/4002/src/pkg/crypto/sha1/sha1block.go#newcode26
src/pkg/crypto/sha1/sha1block.go:26: j := i * 4
j := i << 2 avoids the imul on intel. The compiler should be smarter
about this.

https://codereview.appspot.com/6820096/

https://codereview.appspot.com/6820096/diff/4002/src/pkg/crypto/sha1/sha1block.go

https://codereview.appspot.com/6820096/diff/4002/src/pkg/crypto/sha1/sha1block.go#newcode26
about this.

I tried changing the variables to unsigned to see if it would change
anything. I also tried adding in the shift instead of the mult, but
neither of these two changes affected the results of my benchmarks. I
think it would be safe to leave this alone for now.

https://codereview.appspot.com/6820096/

Fair enough. The compiler should be able to do this for us eventually.

I know you're a Googler, so most of the CLA process does not apply to
you, but have you done whatever is needed on your side ?

https://codereview.appspot.com/6820096/

To be clear, are you approving this patch? Let me know.

We just need to get him into CONTRIBUTORS.

Ian

Yes, LGTM.

Some results from

linux/arm:

benchmark old ns/op new ns/op delta
BenchmarkHash8Bytes 6487 6497 +0.15%
BenchmarkHash1K 69794 70773 +1.40%
BenchmarkHash8K 517529 525604 +1.56%

benchmark old MB/s new MB/s speedup
BenchmarkHash8Bytes 1.23 1.23 1.00x
BenchmarkHash1K 14.67 14.47 0.99x
BenchmarkHash8K 15.83 15.59 0.98x

Wasn't expecting much, and it looks like my expectations were met. This
is within the margin of error.

https://codereview.appspot.com/6820096/

*** Submitted as
http://code.google.com/p/go/source/detail?r=1369d7bb329d ***

crypto/sha1: Make sha-1 do block mixup in place

Benchmarks:

benchmark old ns/op new ns/op delta
BenchmarkHash8Bytes 762 674 -11.55%
BenchmarkHash1K 8791 7375 -16.11%
BenchmarkHash8K 65094 54881 -15.69%

benchmark old MB/s new MB/s speedup
BenchmarkHash8Bytes 10.50 11.86 1.13x
BenchmarkHash1K 116.48 138.84 1.19x
BenchmarkHash8K 125.85 149.27 1.19x

R=dave, rsc, iant
CC=golang-dev
http://codereview.appspot.com/6820096

Committer: Dave Cheney <dave@cheney.net>


http://codereview.appspot.com/6820096/