code review 56990044: An ARM version of sha1block.go with a big improvement i...

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

[dave_cheney.net, 2014-01-28 11:00:30]

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

[dave_cheney.net, 2014-01-28 11:11:25]

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

[Nick Craig-Wood, 2014-02-08 15:21:30]

Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2014-02-10 05:14:53]

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/sha1bl...
File src/pkg/crypto/sha1/sha1block.go (right):

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
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.

[Nick Craig-Wood, 2014-02-10 15:29:56]

On 2014/02/10 05:14:53, dfc wrote:
>
https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
> 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.

[agl1, 2014-02-10 15:44:25]

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/sha1bl...
File src/pkg/crypto/sha1/sha1block_arm.s (right):

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
src/pkg/crypto/sha1/sha1block_arm.s:1: // Copyright 2013 The Go Authors.  All
rights reserved.
2014

[dave_cheney.net, 2014-02-10 20:31:01]

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/
>

[bradfitz, 2014-02-10 20:36:01]

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
File src/pkg/crypto/sha1/sha1block.go (right):

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
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?

[dave_cheney.net, 2014-02-10 20:44:22]

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/
>

[bradfitz, 2014-02-11 07:01:08]

[+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/
>

[adg, 2014-02-11 07:09:05]

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
File src/pkg/crypto/sha1/sha1block.go (right):

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
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?

[adg, 2014-02-11 07:12:51]

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?

[bradfitz, 2014-02-11 07:13:14]

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/
>

[bradfitz, 2014-02-11 07:15:01]

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.
>

[adg, 2014-02-11 07:19:14]

Well it sounds a lot better than spinning up a new builder just to run a
handful of tests.

[nick_craig-wood.com, 2014-02-11 07:45:38]

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

[Nick Craig-Wood, 2014-02-11 09:03:55]

I've corrected the year in the comments - otherwise unchanged

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
File src/pkg/crypto/sha1/sha1block_arm.s (right):

https://codereview.appspot.com/56990044/diff/60001/src/pkg/crypto/sha1/sha1bl...
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.

[agl1, 2014-02-12 18:24:31]

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.

[agl1, 2014-02-12 18:25:07]

*** 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>

[rsc, 2014-02-12 18:38:52]

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.

[bradfitz, 2014-02-12 19:02:34]

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/
>

[Nick Craig-Wood, 2014-02-13 23:28:21]

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!

[dave_cheney.net, 2014-02-13 23:30:01]

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/
>