Code review - Issue 56990044: code review 56990044: An ARM version of sha1block.go with a big improvement i...https://codereview.appspot.com/2014-02-13T23:30:01+00:00rietveld
Message from unknown
2014-01-25T15:48:41+00:00Nick Craig-Woodurn:md5:cec231fb1ad31db289c4cffef3d3ed03
Message from unknown
2014-01-25T15:48:46+00:00Nick Craig-Woodurn:md5:140706fcfa61a1d667969f8ada48ad4d
Message from dave@cheney.net
2014-01-28T11:00:30+00:00dfcurn:md5:4420ba99d51f1c0d6580117c99cf28b0
pandaboard, cortex-a9,
benchmark old ns/op new ns/op delta
BenchmarkHash8Bytes 8769 4218 -51.90%
BenchmarkHash1K 106997 36219 -66.15%
BenchmarkHash8K 792800 261373 -67.03%
benchmark old MB/s new MB/s speedup
BenchmarkHash8Bytes 0.91 1.90 2.09x
BenchmarkHash1K 9.57 28.27 2.95x
BenchmarkHash8K 10.33 31.34 3.03x
Message from dave@cheney.net
2014-01-28T11:11:25+00:00dfcurn:md5:ed38502d3d78a680aa257b1999ecc05d
On 2014/01/28 11:00:30, dfc wrote:
> pandaboard, cortex-a9,
>
> benchmark old ns/op new ns/op delta
> BenchmarkHash8Bytes 8769 4218 -51.90%
> BenchmarkHash1K 106997 36219 -66.15%
> BenchmarkHash8K 792800 261373 -67.03%
>
> benchmark old MB/s new MB/s speedup
> BenchmarkHash8Bytes 0.91 1.90 2.09x
> BenchmarkHash1K 9.57 28.27 2.95x
> BenchmarkHash8K 10.33 31.34 3.03x
dfc@qnap:~/go/src/pkg/crypto/sha1$ cat /proc/cpuinfo
processor : 0
model name : Feroceon 88FR131 rev 1 (v5l)
BogoMIPS : 1980.82
Features : swp half thumb fastmult edsp
CPU implementer : 0x56
CPU architecture: 5TE
CPU variant : 0x2
CPU part : 0x131
CPU revision : 1
Hardware : QNAP TS-119/TS-219
Revision : 0000
Serial : 0000000000000000
dfc@qnap:~/go/src/pkg/crypto/sha1$ ~/go/misc/benchcmp {old,new}.txt
benchmark old ns/op new ns/op delta
BenchmarkHash8Bytes 4568 2129 -53.39%
BenchmarkHash1K 56094 16086 -71.32%
BenchmarkHash8K 417414 113990 -72.69%
benchmark old MB/s new MB/s speedup
BenchmarkHash8Bytes 1.75 3.76 2.15x
BenchmarkHash1K 18.25 63.65 3.49x
BenchmarkHash8K 19.63 71.87 3.66x
Message from unknown
2014-02-08T15:08:52+00:00Nick Craig-Woodurn:md5:b32ea2cbe2cb4737b236ae96f8f763a7
Message from unknown
2014-02-08T15:21:25+00:00Nick Craig-Woodurn:md5:ded19cdfd2d91fcc2b3e137c3c15fbb7
Message from nickcw@gmail.com
2014-02-08T15:21:30+00:00Nick Craig-Woodurn:md5:016f62180d7cd2a4400394f0329b7739
Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from dave@cheney.net
2014-02-10T05:14:53+00:00dfcurn:md5:d0623422eae4b519210fcdde5f9465b8
LGTM.
I don't have any further comments on this CL. I'd like agl and brad to take a look.
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go
File src/pkg/crypto/sha1/sha1block.go (right):
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go#newcode5
src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
We now have no implementations that use sha1block.go.
We have a similar situation with the rc4 implementation which was (and maybe is) broken for a long time. I'll raise a note on golang-dev to discuss the future of this file.
Message from nickcw@gmail.com
2014-02-10T15:29:56+00:00Nick Craig-Woodurn:md5:507b940c4102725682e0a91f80125247
On 2014/02/10 05:14:53, dfc wrote:
> https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go#newcode5
> src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
> We now have no implementations that use sha1block.go.
>
> We have a similar situation with the rc4 implementation which was (and maybe is)
> broken for a long time. I'll raise a note on golang-dev to discuss the future of
> this file.
Same situation in crypto/md5 too.
IMHO It would be nice to keep these implementations if we can stop them bit rotting somehow which will make future architecture ports a lot easier.
Message from agl@golang.org
2014-02-10T15:44:25+00:00agl1urn:md5:6de536faba0dfbf3cb246754199d09c9
LGTM.
I'll ask adg whether we can, additionally, build the packages with a "noasm" tag on one of the builders to ensure the health of the generic code.
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block_arm.s
File src/pkg/crypto/sha1/sha1block_arm.s (right):
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block_arm.s#newcode1
src/pkg/crypto/sha1/sha1block_arm.s:1: // Copyright 2013 The Go Authors. All rights reserved.
2014
Message from dave@cheney.net
2014-02-10T20:31:01+00:00dfcurn:md5:0ac3edfc6f83bf6ea3fd1a9bcf6ec77f
On Tue, Feb 11, 2014 at 2:29 AM, <nickcw@gmail.com> wrote:
> On 2014/02/10 05:14:53, dfc wrote:
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go#newcode5
>
>> src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
>> We now have no implementations that use sha1block.go.
>>
>
> We have a similar situation with the rc4 implementation which was (and
>>
> maybe is)
>
>> broken for a long time. I'll raise a note on golang-dev to discuss the
>>
> future of
>
>> this file.
>>
>
> Same situation in crypto/md5 too.
>
> IMHO It would be nice to keep these implementations if we can stop them
> bit rotting somehow which will make future architecture ports a lot
> easier.
>
>
I believe that with the mips support in gccgo, and the upcoming arm64
support in the same, these implementations are used, but waiting for those
changes to filter down to gccgo to get tested isn't that fair on Iant.
>
> https://codereview.appspot.com/56990044/
>
Message from bradfitz@golang.org
2014-02-10T20:36:01+00:00bradfitzurn:md5:403cac824a2589aa5fe5b003d4cea60e
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go
File src/pkg/crypto/sha1/sha1block.go (right):
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go#newcode5
src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
On 2014/02/10 05:14:53, dfc wrote:
> We now have no implementations that use sha1block.go.
>
> We have a similar situation with the rc4 implementation which was (and maybe is)
> broken for a long time. I'll raise a note on golang-dev to discuss the future of
> this file.
Let's remove this +build line so this file always builds, then rename the "func block" to "func genblock" below, and make a new file that's sha1block_gen.go that is +build !amd64,!386,!arm and simply does:
var block = genblock
Then we can write tests for genblock that run on all builders. So even amd64,386,arm test both the assembly versions and the Go versions.
The linker will discard func genblock from real binaries.
Thoughts?
Message from dave@cheney.net
2014-02-10T20:44:22+00:00dfcurn:md5:21b359931f49771dd29ee48a148d6871
SGTM. But please let's do this for the three (at the moment) packages
affected in a followup CL There is (was) a pattern that can be follows in
the bytes package.
On Tue, Feb 11, 2014 at 7:36 AM, <bradfitz@golang.org> wrote:
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go
> File src/pkg/crypto/sha1/sha1block.go (right):
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go#newcode5
> src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
> On 2014/02/10 05:14:53, dfc wrote:
>
>> We now have no implementations that use sha1block.go.
>>
>
> We have a similar situation with the rc4 implementation which was (and
>>
> maybe is)
>
>> broken for a long time. I'll raise a note on golang-dev to discuss the
>>
> future of
>
>> this file.
>>
>
> Let's remove this +build line so this file always builds, then rename
> the "func block" to "func genblock" below, and make a new file that's
> sha1block_gen.go that is +build !amd64,!386,!arm and simply does:
>
> var block = genblock
>
> Then we can write tests for genblock that run on all builders. So even
> amd64,386,arm test both the assembly versions and the Go versions.
>
> The linker will discard func genblock from real binaries.
>
> Thoughts?
>
> https://codereview.appspot.com/56990044/
>
Message from bradfitz@golang.org
2014-02-11T07:01:08+00:00bradfitzurn:md5:fd4aff703d3abe686b44b149cfe01c08
[+adg. this thread]
On Mon, Feb 10, 2014 at 12:36 PM, <bradfitz@golang.org> wrote:
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go
> File src/pkg/crypto/sha1/sha1block.go (right):
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go#newcode5
> src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
> On 2014/02/10 05:14:53, dfc wrote:
>
>> We now have no implementations that use sha1block.go.
>>
>
> We have a similar situation with the rc4 implementation which was (and
>>
> maybe is)
>
>> broken for a long time. I'll raise a note on golang-dev to discuss the
>>
> future of
>
>> this file.
>>
>
> Let's remove this +build line so this file always builds, then rename
> the "func block" to "func genblock" below, and make a new file that's
> sha1block_gen.go that is +build !amd64,!386,!arm and simply does:
>
> var block = genblock
>
> Then we can write tests for genblock that run on all builders. So even
> amd64,386,arm test both the assembly versions and the Go versions.
>
> The linker will discard func genblock from real binaries.
>
> Thoughts?
>
> https://codereview.appspot.com/56990044/
>
Message from adg@golang.org
2014-02-11T07:09:05+00:00adgurn:md5:9a8f565a55db09914bba9bc37b462798
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go
File src/pkg/crypto/sha1/sha1block.go (right):
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block.go#newcode5
src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
On 2014/02/10 20:36:01, bradfitz wrote:
> On 2014/02/10 05:14:53, dfc wrote:
> > We now have no implementations that use sha1block.go.
> >
> > We have a similar situation with the rc4 implementation which was (and maybe
> is)
> > broken for a long time. I'll raise a note on golang-dev to discuss the future
> of
> > this file.
>
> Let's remove this +build line so this file always builds, then rename the "func
> block" to "func genblock" below, and make a new file that's sha1block_gen.go
> that is +build !amd64,!386,!arm and simply does:
>
> var block = genblock
>
> Then we can write tests for genblock that run on all builders. So even
> amd64,386,arm test both the assembly versions and the Go versions.
>
> The linker will discard func genblock from real binaries.
>
> Thoughts?
So the tests will call both block and genblock?
Message from adg@golang.org
2014-02-11T07:12:51+00:00adgurn:md5:aca769f1d5173daeb0115602240a2a6c
On 11 February 2014 18:09, <adg@golang.org> wrote:
> So the tests will call both block and genblock?
As in, each test will have to know to call each function?
Message from bradfitz@golang.org
2014-02-11T07:13:14+00:00bradfitzurn:md5:fe1302d330ba77ed3f7f808934ecd162
Yes. I wrote as such in the email. ("So even ...")
Seems better than maintaining a new heavy builder.
On Feb 10, 2014 11:09 PM, <adg@golang.org> wrote:
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go
> File src/pkg/crypto/sha1/sha1block.go (right):
>
> https://codereview.appspot.com/56990044/diff/60001/src/
> pkg/crypto/sha1/sha1block.go#newcode5
> src/pkg/crypto/sha1/sha1block.go:5: // +build !amd64,!386,!arm
> On 2014/02/10 20:36:01, bradfitz wrote:
>
>> On 2014/02/10 05:14:53, dfc wrote:
>> > We now have no implementations that use sha1block.go.
>> >
>> > We have a similar situation with the rc4 implementation which was
>>
> (and maybe
>
>> is)
>> > broken for a long time. I'll raise a note on golang-dev to discuss
>>
> the future
>
>> of
>> > this file.
>>
>
> Let's remove this +build line so this file always builds, then rename
>>
> the "func
>
>> block" to "func genblock" below, and make a new file that's
>>
> sha1block_gen.go
>
>> that is +build !amd64,!386,!arm and simply does:
>>
>
> var block = genblock
>>
>
> Then we can write tests for genblock that run on all builders. So
>>
> even
>
>> amd64,386,arm test both the assembly versions and the Go versions.
>>
>
> The linker will discard func genblock from real binaries.
>>
>
> Thoughts?
>>
>
> So the tests will call both block and genblock?
>
> https://codereview.appspot.com/56990044/
>
Message from bradfitz@golang.org
2014-02-11T07:15:01+00:00bradfitzurn:md5:01cf539e187d39f483c557e66d0ad0b4
Sure. Is that hard? Or just test the generic one a tiny bit. Main fear is
it failing to compile. Doesn't need the same level of test coverage. It's a
smoke test.
On Feb 10, 2014 11:12 PM, "Andrew Gerrand" <adg@golang.org> wrote:
>
> On 11 February 2014 18:09, <adg@golang.org> wrote:
>
>> So the tests will call both block and genblock?
>
>
> As in, each test will have to know to call each function?
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
Message from adg@golang.org
2014-02-11T07:19:14+00:00adgurn:md5:41d28d3356a33eb5d2ae2a451d7dc067
Well it sounds a lot better than spinning up a new builder just to run a
handful of tests.
Message from nick@craig-wood.com
2014-02-11T07:45:38+00:00nick_craig-wood.comurn:md5:bfb92fd488789f751c35712cc127fbf9
On 11/02/14 07:18, Andrew Gerrand wrote:
> Well it sounds a lot better than spinning up a new builder just to run a
> handful of tests.
I don't know how hard it is to run a new builder, but IMHO the "noasm"
tag suggestion tests exactly what we want to test - namely the compile
and test without the optimised assembler, whereas Brad's suggestions
risks a different sort of bit rot with duplicated tests.
--
Nick Craig-Wood <nick@craig-wood.com> -- http://www.craig-wood.com/nick
Message from unknown
2014-02-11T09:01:52+00:00Nick Craig-Woodurn:md5:8ac5a3f5afcd602837a8e0d20c8cbdb5
Message from nickcw@gmail.com
2014-02-11T09:03:55+00:00Nick Craig-Woodurn:md5:0c5ef728a73efe95ad1a21968b84385a
I've corrected the year in the comments - otherwise unchanged
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block_arm.s
File src/pkg/crypto/sha1/sha1block_arm.s (right):
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1block_arm.s#newcode1
src/pkg/crypto/sha1/sha1block_arm.s:1: // Copyright 2013 The Go Authors. All rights reserved.
On 2014/02/10 15:44:26, agl1 wrote:
> 2014
Done.
Message from agl@golang.org
2014-02-12T18:24:31+00:00agl1urn:md5:8a078786ecbd7ac15fb39d3764ede979
I'm not sure what the answer to testing the non-asm code is, but that's a more general problem so I'll land this CL on the weight of its LGTMs.
Message from agl@golang.org
2014-02-12T18:25:07+00:00agl1urn:md5:4b143fcce8332534eb66d3d11ab8a169
*** Submitted as https://code.google.com/p/go/source/detail?r=d1ca79bd75ee ***
An ARM version of sha1block.go with a big improvement in throughput
(up to 2.8x).
This is a partially unrolled version which performs better for small
hashes and only sacrifices a small amount of ultimate speed to a fully
unrolled version which uses 5k of code.
Code size
Before 1636 bytes
After 1880 bytes
15% larger
Benchmarks on Samsung Exynos 5 ARMv7 Chromebook
benchmark old ns/op new ns/op delta
BenchmarkHash8Bytes 1907 1136 -40.43%
BenchmarkHash1K 20280 7547 -62.79%
BenchmarkHash8K 148469 52576 -64.59%
benchmark old MB/s new MB/s speedup
BenchmarkHash8Bytes 4.19 7.04 1.68x
BenchmarkHash1K 50.49 135.68 2.69x
BenchmarkHash8K 55.18 155.81 2.82x
LGTM=dave, agl
R=dave, bradfitz, agl, adg, nick
CC=golang-codereviews
https://codereview.appspot.com/56990044
Committer: Adam Langley <agl@golang.org>
Message from rsc@golang.org
2014-02-12T18:38:52+00:00rscurn:md5:c04009a383f9fe98b06e9a95a675971f
For future CLs, please make the first line a summary of what's being changed. For this CL it would have been something like
crypto/sha1: add arm assembly
Thanks.
Message from bradfitz@golang.org
2014-02-12T19:02:34+00:00bradfitzurn:md5:ca0f5795faf1477e34515ce57f72226a
I've sent https://codereview.appspot.com/62270043/ for testing.
If non-objectionable, we can do that pattern elsewhere too.
On Wed, Feb 12, 2014 at 10:25 AM, <agl@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=d1ca79bd75ee ***
>
>
> An ARM version of sha1block.go with a big improvement in throughput
> (up to 2.8x).
>
> This is a partially unrolled version which performs better for small
> hashes and only sacrifices a small amount of ultimate speed to a fully
> unrolled version which uses 5k of code.
>
> Code size
>
> Before 1636 bytes
> After 1880 bytes
> 15% larger
>
> Benchmarks on Samsung Exynos 5 ARMv7 Chromebook
>
> benchmark old ns/op new ns/op delta
> BenchmarkHash8Bytes 1907 1136 -40.43%
> BenchmarkHash1K 20280 7547 -62.79%
> BenchmarkHash8K 148469 52576 -64.59%
>
> benchmark old MB/s new MB/s speedup
> BenchmarkHash8Bytes 4.19 7.04 1.68x
> BenchmarkHash1K 50.49 135.68 2.69x
> BenchmarkHash8K 55.18 155.81 2.82x
>
> LGTM=dave, agl
> R=dave, bradfitz, agl, adg, nick
> CC=golang-codereviews
> https://codereview.appspot.com/56990044
>
> Committer: Adam Langley <agl@golang.org>
>
>
> https://codereview.appspot.com/56990044/
>
Message from nickcw@gmail.com
2014-02-13T23:28:21+00:00Nick Craig-Woodurn:md5:c5b1217ae1d7e19cde52baf426b32117
On 2014/02/12 18:38:52, rsc wrote:
> For future CLs, please make the first line a summary of what's being changed.
> For this CL it would have been something like
>
> crypto/sha1: add arm assembly
>
> Thanks.
Arg - sorry!
Originally the title was "crypto/sha1: native ARM assembler version" but I think I must have changed it without noticing when I edited the commit message in the web interface.
I'll pay more attention next time!
Message from dave@cheney.net
2014-02-13T23:30:01+00:00dfcurn:md5:f647eca0cf24791102e3795dc8dbba2d
protip: if you edit the description via the web interface then you should
remember to edit both the description field and the title field.
I normally do hg change NNN && hg mail NNN
On Fri, Feb 14, 2014 at 10:28 AM, <nickcw@gmail.com> wrote:
> On 2014/02/12 18:38:52, rsc wrote:
>
>> For future CLs, please make the first line a summary of what's being
>>
> changed.
>
>> For this CL it would have been something like
>>
>
> crypto/sha1: add arm assembly
>>
>
> Thanks.
>>
>
> Arg - sorry!
>
> Originally the title was "crypto/sha1: native ARM assembler version" but
> I think I must have changed it without noticing when I edited the commit
> message in the web interface.
>
> I'll pay more attention next time!
>
> https://codereview.appspot.com/56990044/
>