Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1045)

Issue 6820096: code review 6820096: crypto/sha1: Make sha-1 do block mixup in place (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by notcarl
Modified:
11 years, 4 months ago
Reviewers:
CC:
dfc, rsc, iant2, golang-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r c33545ae0ec0 https://code.google.com/p/go/ #

Patch Set 3 : diff -r c33545ae0ec0 https://code.google.com/p/go/ #

Total comments: 6

Patch Set 4 : diff -r c33545ae0ec0 https://code.google.com/p/go/ #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -14 lines) Patch
M src/pkg/crypto/sha1/sha1block.go View 1 2 3 2 chunks +28 lines, -14 lines 1 comment Download

Messages

Total messages: 21
notcarl
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
11 years, 4 months ago (2012-11-06 01:46:04 UTC) #1
notcarl
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-06 02:12:48 UTC) #2
dfc
On 2012/11/06 02:12:48, notcarl wrote: > Hello mailto:golang-dev@googlegroups.com (cc: mailto:golang-dev@googlegroups.com), > > Please take another ...
11 years, 4 months ago (2012-11-06 04:01:37 UTC) #3
dfc
https://codereview.appspot.com/6820096/diff/6001/src/pkg/crypto/sha1/sha1block.go File src/pkg/crypto/sha1/sha1block.go (right): https://codereview.appspot.com/6820096/diff/6001/src/pkg/crypto/sha1/sha1block.go#newcode35 src/pkg/crypto/sha1/sha1block.go:35: for i := 0; i < 16; i++ { ...
11 years, 4 months ago (2012-11-06 04:06:47 UTC) #4
rsc
looks good to me. will leave for dfc to lgtm and submit https://codereview.appspot.com/6820096/diff/6001/src/pkg/crypto/sha1/sha1block.go File src/pkg/crypto/sha1/sha1block.go ...
11 years, 4 months ago (2012-11-06 18:52:06 UTC) #5
notcarl
Thanks for the quick review! https://codereview.appspot.com/6820096/diff/6001/src/pkg/crypto/sha1/sha1block.go File src/pkg/crypto/sha1/sha1block.go (right): https://codereview.appspot.com/6820096/diff/6001/src/pkg/crypto/sha1/sha1block.go#newcode39 src/pkg/crypto/sha1/sha1block.go:39: t := a5 + ...
11 years, 4 months ago (2012-11-06 19:02:33 UTC) #6
rsc
>> i&0xf will get you something here. > > I am not sure I understand. ...
11 years, 4 months ago (2012-11-06 19:08:54 UTC) #7
notcarl
Hello dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-06 19:27:01 UTC) #8
notcarl
On 2012/11/06 19:08:54, rsc wrote: > >> i&0xf will get you something here. > > ...
11 years, 4 months ago (2012-11-06 19:31:11 UTC) #9
rsc
Please put the current benchcmp numbers in the CL description. Leaving for Dave. Thanks. Russ
11 years, 4 months ago (2012-11-06 19:38:57 UTC) #10
notcarl
On 2012/11/06 19:38:57, rsc wrote: > Please put the current benchcmp numbers in the CL ...
11 years, 4 months ago (2012-11-06 19:57:05 UTC) #11
dfc
hmmm, i can't replicate your benchmark results. linux/amd64: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz benchmark ...
11 years, 4 months ago (2012-11-06 21:46:54 UTC) #12
notcarl
$ cat /proc/cpuinfo processor : 0 vendor_id : GenuineIntel cpu family : 6 model : ...
11 years, 4 months ago (2012-11-06 21:50:20 UTC) #13
dfc
Looks pretty good to me as well. I'll run some benchmarks on 32 bit platforms ...
11 years, 4 months ago (2012-11-06 22:04:24 UTC) #14
notcarl
On 2012/11/06 22:04:24, dfc wrote: > Looks pretty good to me as well. I'll run ...
11 years, 4 months ago (2012-11-06 22:51:01 UTC) #15
dfc
Fair enough. The compiler should be able to do this for us eventually. I know ...
11 years, 4 months ago (2012-11-06 23:07:38 UTC) #16
iant2
On Tue, Nov 6, 2012 at 3:07 PM, <dave@cheney.net> wrote: > Fair enough. The compiler ...
11 years, 4 months ago (2012-11-06 23:18:33 UTC) #17
dfc
Yes, LGTM. On Wed, Nov 7, 2012 at 10:18 AM, Ian Lance Taylor <iant@google.com> wrote: ...
11 years, 4 months ago (2012-11-06 23:18:52 UTC) #18
notcarl
On 2012/11/06 23:07:38, dfc wrote: > Fair enough. The compiler should be able to do ...
11 years, 4 months ago (2012-11-06 23:23:11 UTC) #19
dfc
Some results from linux/arm: benchmark old ns/op new ns/op delta BenchmarkHash8Bytes 6487 6497 +0.15% BenchmarkHash1K ...
11 years, 4 months ago (2012-11-06 23:24:36 UTC) #20
dfc
11 years, 4 months ago (2012-11-07 02:41:18 UTC) #21
*** 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>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b