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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 6 months ago by Nick Craig-Wood
Modified:
7 years, 5 months ago
Reviewers:
agl1, rsc, dfc
CC:
dfc, bradfitz, agl1, adg, nick_craig-wood.com, golang-codereviews
Visibility:
Public.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -2 lines) Patch
M src/pkg/crypto/sha1/sha1block.go View 1 1 chunk +1 line, -1 line 0 comments Download
A src/pkg/crypto/sha1/sha1block_arm.s View 1 2 3 4 1 chunk +217 lines, -0 lines 0 comments Download
M src/pkg/crypto/sha1/sha1block_decl.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 23
dfc
pandaboard, cortex-a9, benchmark old ns/op new ns/op delta BenchmarkHash8Bytes 8769 4218 -51.90% BenchmarkHash1K 106997 36219 ...
7 years, 6 months ago (2014-01-28 11:00:30 UTC) #1
dfc
On 2014/01/28 11:00:30, dfc wrote: > pandaboard, cortex-a9, > > benchmark old ns/op new ns/op ...
7 years, 6 months ago (2014-01-28 11:11:25 UTC) #2
Nick Craig-Wood
Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
7 years, 5 months ago (2014-02-08 15:21:30 UTC) #3
dfc
LGTM. I don't have any further comments on this CL. I'd like agl and brad ...
7 years, 5 months ago (2014-02-10 05:14:53 UTC) #4
Nick Craig-Wood
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 ...
7 years, 5 months ago (2014-02-10 15:29:56 UTC) #5
agl1
LGTM. I'll ask adg whether we can, additionally, build the packages with a "noasm" tag ...
7 years, 5 months ago (2014-02-10 15:44:25 UTC) #6
dfc
On Tue, Feb 11, 2014 at 2:29 AM, <nickcw@gmail.com> wrote: > On 2014/02/10 05:14:53, dfc ...
7 years, 5 months ago (2014-02-10 20:31:01 UTC) #7
bradfitz
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: > ...
7 years, 5 months ago (2014-02-10 20:36:01 UTC) #8
dfc
SGTM. But please let's do this for the three (at the moment) packages affected in ...
7 years, 5 months ago (2014-02-10 20:44:22 UTC) #9
bradfitz
[+adg. this thread] On Mon, Feb 10, 2014 at 12:36 PM, <bradfitz@golang.org> wrote: > > ...
7 years, 5 months ago (2014-02-11 07:01:08 UTC) #10
adg
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: > ...
7 years, 5 months ago (2014-02-11 07:09:05 UTC) #11
adg
On 11 February 2014 18:09, <adg@golang.org> wrote: > So the tests will call both block ...
7 years, 5 months ago (2014-02-11 07:12:51 UTC) #12
bradfitz
Yes. I wrote as such in the email. ("So even ...") Seems better than maintaining ...
7 years, 5 months ago (2014-02-11 07:13:14 UTC) #13
bradfitz
Sure. Is that hard? Or just test the generic one a tiny bit. Main fear ...
7 years, 5 months ago (2014-02-11 07:15:01 UTC) #14
adg
Well it sounds a lot better than spinning up a new builder just to run ...
7 years, 5 months ago (2014-02-11 07:19:14 UTC) #15
nick_craig-wood.com
On 11/02/14 07:18, Andrew Gerrand wrote: > Well it sounds a lot better than spinning ...
7 years, 5 months ago (2014-02-11 07:45:38 UTC) #16
Nick Craig-Wood
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 ...
7 years, 5 months ago (2014-02-11 09:03:55 UTC) #17
agl1
I'm not sure what the answer to testing the non-asm code is, but that's a ...
7 years, 5 months ago (2014-02-12 18:24:31 UTC) #18
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=d1ca79bd75ee *** An ARM version of sha1block.go with a big improvement in ...
7 years, 5 months ago (2014-02-12 18:25:07 UTC) #19
rsc
For future CLs, please make the first line a summary of what's being changed. For ...
7 years, 5 months ago (2014-02-12 18:38:52 UTC) #20
bradfitz
I've sent https://codereview.appspot.com/62270043/ for testing. If non-objectionable, we can do that pattern elsewhere too. On ...
7 years, 5 months ago (2014-02-12 19:02:34 UTC) #21
Nick Craig-Wood
On 2014/02/12 18:38:52, rsc wrote: > For future CLs, please make the first line a ...
7 years, 5 months ago (2014-02-13 23:28:21 UTC) #22
dfc
7 years, 5 months ago (2014-02-13 23:30:01 UTC) #23
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.

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