|
|
Created:
10 years, 3 months ago by Nick Craig-Wood Modified:
10 years, 1 month ago CC:
dave_cheney.net, bradfitz, agl1, adg, nick_craig-wood.com, golang-codereviews Visibility:
Public. |
DescriptionAn 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
Patch Set 1 #Patch Set 2 : diff -r 54a5513d9d6a https://code.google.com/p/go #Patch Set 3 : diff -r 54a5513d9d6a https://code.google.com/p/go #Patch Set 4 : diff -r 54a5513d9d6a https://code.google.com/p/go #
Total comments: 5
Patch Set 5 : diff -r 54a5513d9d6a https://code.google.com/p/go #
MessagesTotal messages: 23
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
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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?
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
[+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/ >
Sign in to reply to this message.
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?
Sign in to reply to this message.
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?
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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. >
Sign in to reply to this message.
Well it sounds a lot better than spinning up a new builder just to run a handful of tests.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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!
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
|