code review 7541045: crypto/cipher: Use builtin copy instead of manually cop...

crypto/cipher: Use builtin copy instead of manually copying values. Updated CryptBlocks method of cbcEncrypter and cbcDecrypter.

[fasmat, 2013-03-10 23:21:03]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[nigeltao, 2013-03-11 22:55:57]

This change is probably fine, but benchmark numbers would be good, and agl should probably review this in case there is any crypto subtlety.

[agl1, 2013-03-12 14:48:33]

https://codereview.appspot.com/7541045/diff/4001/src/pkg/crypto/cipher/cbc.go File src/pkg/crypto/cipher/cbc.go (right): https://codereview.appspot.com/7541045/diff/4001/src/pkg/crypto/cipher/cbc.go... src/pkg/crypto/cipher/cbc.go:56: copy(dst, x.iv) This looks fine. (This code was written before copy() existed.) https://codereview.appspot.com/7541045/diff/4001/src/pkg/crypto/cipher/cbc.go... src/pkg/crypto/cipher/cbc.go:88: copy(dst, x.tmp) I think this fails if dst==src. It may be correct with these two new lines switched however.

[fasmat, 2013-03-12 14:52:13]

> https://codereview.appspot.com/7541045/diff/4001/src/pkg/crypto/cipher/cbc.go... > src/pkg/crypto/cipher/cbc.go:88: copy(dst, x.tmp) > I think this fails if dst==src. It may be correct with these two new lines > switched however. You could be right. I just did ./all.bash and crypto/tls failed testing. This could be the reason. I will check it in some hours and do some benchmarking.

[fasmat, 2013-03-12 22:13:39]

Hello agl@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[agl1, 2013-03-12 22:17:57]

https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc.go File src/pkg/crypto/cipher/cbc.go (right): https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc.g... src/pkg/crypto/cipher/cbc.go:54: dst[i] = x.iv[i] I'm not sure why this line is here. Worthy of a comment at least. https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... File src/pkg/crypto/cipher/cbc_aes_test.go (right): https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... src/pkg/crypto/cipher/cbc_aes_test.go:100: encrypter := cipher.NewCBCEncrypter(c, commonIV) There's no use of b.N here, not ResetTimer after the setup. https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... src/pkg/crypto/cipher/cbc_aes_test.go:108: if !bytes.Equal(a, p) { No need for this in a benchmark.

[fasmat, 2013-03-12 22:19:10]

On 2013/03/12 22:13:39, fasmat wrote: > Hello mailto:agl@golang.org (cc: mailto:golang-dev@googlegroups.com), > > Please take another look. I benchmarked the new code to the old one. Interestingly the version with copy is about 20% slower, at least on my machine. Off-Topic: Is it really necessary to submit a new version of the patch with hg change 'id' and hg mail 'id' as described in http://golang.org/doc/contribute.html ? It seams to submit the patch set twice.

[fasmat, 2013-03-12 22:37:13]

https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc.go File src/pkg/crypto/cipher/cbc.go (right): https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc.g... src/pkg/crypto/cipher/cbc.go:54: dst[i] = x.iv[i] On 2013/03/12 22:17:57, agl1 wrote: > I'm not sure why this line is here. Worthy of a comment at least. Sorry, my mistake. I forgot to remove it after testing.

[fasmat, 2013-03-12 22:38:20]

https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... File src/pkg/crypto/cipher/cbc_aes_test.go (right): https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... src/pkg/crypto/cipher/cbc_aes_test.go:100: encrypter := cipher.NewCBCEncrypter(c, commonIV) On 2013/03/12 22:17:57, agl1 wrote: > There's no use of b.N here, not ResetTimer after the setup. Resolved in my newest patch set. https://codereview.appspot.com/7541045/diff/15001/src/pkg/crypto/cipher/cbc_a... src/pkg/crypto/cipher/cbc_aes_test.go:108: if !bytes.Equal(a, p) { On 2013/03/12 22:17:57, agl1 wrote: > No need for this in a benchmark. Removed

[fasmat, 2013-03-12 22:53:09]

With the new benchmark function I get similar results. The copy version of the code is ~20% slower than the original version.

[dave_cheney.net, 2013-03-12 22:58:30]

I adjusted the size of the buffer being encrypted to 1<<20 to give more iterations, here are the results on a core i5 linux/amd64 lucky(~/go/src/pkg/crypto/cipher) % ~/go/misc/benchcmp {old,new}.txt benchmark old ns/op new ns/op delta BenchmarkCBC_AES 13840063 13915572 +0.55%

[fasmat, 2013-03-12 23:11:37]

> lucky(~/go/src/pkg/crypto/cipher) % ~/go/misc/benchcmp {old,new}.txt > benchmark old ns/op new ns/op delta > BenchmarkCBC_AES 13840063 13915572 +0.55% I also have a core i5 running linux/amd64. My results: benchmark old ns/op new ns/op delta BenchmarkCBC_AES 13228520 17018574 +28.65% Don't know why copy is so slow on my machine.

[dave_cheney.net, 2013-03-12 23:12:45]

To be clear, I don't think this change is a bad one. But I would suggest leaving this til post go 1.1 as it doesn't appear to pay it's way. On Wed, Mar 13, 2013 at 10:11 AM, <fasching.matthias@gmail.com> wrote: >> lucky(~/go/src/pkg/crypto/cipher) % ~/go/misc/benchcmp {old,new}.txt >> benchmark old ns/op new ns/op delta >> BenchmarkCBC_AES 13840063 13915572 +0.55% > > > I also have a core i5 running linux/amd64. My results: > > benchmark old ns/op new ns/op delta > BenchmarkCBC_AES 13228520 17018574 +28.65% > > Don't know why copy is so slow on my machine. > > https://codereview.appspot.com/7541045/ 

[fasmat, 2013-03-13 09:29:19]

OK, then I will start on another update to crypto/cipher, which is not related to this one. On 2013/03/12 23:12:45, dfc wrote: > To be clear, I don't think this change is a bad one. But I would > suggest leaving this til post go 1.1 as it doesn't appear to pay it's > way.

[rsc, 2013-12-18 20:14:34]

R=close (timed out)