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

Issue 6850043: code review 6850043: crypto/md5: add arm to md5block special case (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by dfc
Modified:
9 years, 7 months ago
Reviewers:
r, minux1, golang-dev
Visibility:
Public.

Description

crypto/md5: add arm to md5block special case benchmark old ns/op new ns/op delta BenchmarkHash8Bytes 4159 3346 -19.55% BenchmarkHash1K 24571 11755 -52.16% BenchmarkHash8K 166339 70581 -57.57% benchmark old MB/s new MB/s speedup BenchmarkHash8Bytes 1.92 2.39 1.24x BenchmarkHash1K 41.67 87.11 2.09x BenchmarkHash8K 49.25 116.06 2.36x

Patch Set 1 #

Patch Set 2 : diff -r fc4e67d82024 https://code.google.com/p/go #

Patch Set 3 : diff -r fc4e67d82024 https://code.google.com/p/go #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M src/pkg/crypto/md5/md5block.go View 1 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 11
dfc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
9 years, 7 months ago (2012-11-13 23:55:22 UTC) #1
r
http://codereview.appspot.com/6850043/diff/5001/src/pkg/crypto/md5/md5block.go File src/pkg/crypto/md5/md5block.go (right): http://codereview.appspot.com/6850043/diff/5001/src/pkg/crypto/md5/md5block.go#newcode24 src/pkg/crypto/md5/md5block.go:24: X = (*[16]uint32)(unsafe.Pointer(&p[0])) can an ARM do unaligned fetches?
9 years, 7 months ago (2012-11-14 00:06:03 UTC) #2
dfc
On 2012/11/14 00:06:03, r wrote: > http://codereview.appspot.com/6850043/diff/5001/src/pkg/crypto/md5/md5block.go > File src/pkg/crypto/md5/md5block.go (right): > > http://codereview.appspot.com/6850043/diff/5001/src/pkg/crypto/md5/md5block.go#newcode24 > ...
9 years, 7 months ago (2012-11-14 00:07:00 UTC) #3
minux1
On Wed, Nov 14, 2012 at 8:07 AM, <dave@cheney.net> wrote: > On 2012/11/14 00:06:03, r ...
9 years, 7 months ago (2012-11-14 11:44:20 UTC) #4
dfc
On 2012/11/14 11:44:20, minux wrote: > On Wed, Nov 14, 2012 at 8:07 AM, <mailto:dave@cheney.net> ...
9 years, 7 months ago (2012-11-15 01:04:25 UTC) #5
minux1
On Thursday, November 15, 2012, wrote: > On 2012/11/14 11:44:20, minux wrote: > >> On ...
9 years, 7 months ago (2012-11-15 03:50:20 UTC) #6
dfc
Yeah, i've been checking that, I can't run my pandaboard on setting 4 (the kernel ...
9 years, 7 months ago (2012-11-15 04:19:19 UTC) #7
dfc
Thank you, your additional test fails on ARMv5. === RUN TestGolden --- FAIL: TestGolden (0.00 ...
9 years, 7 months ago (2012-11-15 07:26:10 UTC) #8
dfc
*** Abandoned ***
9 years, 7 months ago (2012-11-15 07:27:10 UTC) #9
minux1
However, i think you can do something like this to speed up the common case ...
9 years, 7 months ago (2012-11-15 07:36:25 UTC) #10
dfc
9 years, 7 months ago (2012-11-15 07:44:41 UTC) #11
SGTM. I think this pattern is also valid for other hashes in the tree. 

On 15/11/2012, at 18:36, minux <minux.ma@gmail.com> wrote:

> However, i think you can do something like this to speed up the common case
where the buffer
> does align to 4-byte boundary:
> 
> diff -r 591fc8a0131a src/pkg/crypto/md5/md5block.go
> --- a/src/pkg/crypto/md5/md5block.go    Wed Nov 14 22:04:03 2012 -0800
> +++ b/src/pkg/crypto/md5/md5block.go    Thu Nov 15 15:32:51 2012 +0800
> @@ -22,6 +22,8 @@
>                         // less code and run 1.3x faster if we take advantage
of that.
>                         // My apologies.
>                         X = (*[16]uint32)(unsafe.Pointer(&p[0]))
> +               } else if uintptr(unsafe.Pointer(&p[0])) & 3 == 0 {
> +                       X = (*[16]uint32)(unsafe.Pointer(&p[0]))
>                 } else {
>                         X = &xbuf
>                         j := 0
> diff -r 591fc8a0131a src/pkg/crypto/md5/gen.go
> --- a/src/pkg/crypto/md5/gen.go Wed Nov 14 22:04:03 2012 -0800
> +++ b/src/pkg/crypto/md5/gen.go Thu Nov 15 15:32:51 2012 +0800
> @@ -203,6 +203,8 @@
>                         // less code and run 1.3x faster if we take advantage
of that.
>                         // My apologies.
>                         X = (*[16]uint32)(unsafe.Pointer(&p[0]))
> +               } else if uintptr(unsafe.Pointer(&p[0])) & 3 == 0 {
> +                       X = (*[16]uint32)(unsafe.Pointer(&p[0]))
>                 } else {
>                         X = &xbuf
>                         j := 0
> 
> Just don't forget to change gen.go.
> I think we can get the same nice benchmark numbers this way
> However, I propose we also add a benchmark for unaligned writes,
> what do you think?
Sign in to reply to this message.

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