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

Issue 11648043: code review 11648043: crypto/md5: native arm assembler version (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years ago by Nick Craig-Wood
Modified:
8 years, 9 months ago
Reviewers:
dfc, rsc
CC:
golang-dev, dfc, bradfitz, rsc, ajstarks, minux1, remyoudompheng
Visibility:
Public.

Description

crypto/md5: native arm assembler version An ARM version of md5block.go with a big improvement in throughput (up to 2.5x) and a reduction in object size (21%). Code size Before 3100 bytes After 2424 bytes 21% smaller Benchmarks on Rasperry Pi benchmark old ns/op new ns/op delta BenchmarkHash8Bytes 11703 6636 -43.30% BenchmarkHash1K 38057 21881 -42.50% BenchmarkHash8K 208131 142735 -31.42% BenchmarkHash8BytesUnaligned 11457 6570 -42.66% BenchmarkHash1KUnaligned 69334 26841 -61.29% BenchmarkHash8KUnaligned 455120 182223 -59.96% benchmark old MB/s new MB/s speedup BenchmarkHash8Bytes 0.68 1.21 1.78x BenchmarkHash1K 26.91 46.80 1.74x BenchmarkHash8K 39.36 57.39 1.46x BenchmarkHash8BytesUnaligned 0.70 1.22 1.74x BenchmarkHash1KUnaligned 14.77 38.15 2.58x BenchmarkHash8KUnaligned 18.00 44.96 2.50x benchmark old allocs new allocs delta BenchmarkHash8Bytes 1 0 -100.00% BenchmarkHash1K 2 0 -100.00% BenchmarkHash8K 2 0 -100.00% BenchmarkHash8BytesUnaligned 1 0 -100.00% BenchmarkHash1KUnaligned 2 0 -100.00% BenchmarkHash8KUnaligned 2 0 -100.00% benchmark old bytes new bytes delta BenchmarkHash8Bytes 64 0 -100.00% BenchmarkHash1K 128 0 -100.00% BenchmarkHash8K 128 0 -100.00% BenchmarkHash8BytesUnaligned 64 0 -100.00% BenchmarkHash1KUnaligned 128 0 -100.00% BenchmarkHash8KUnaligned 128 0 -100.00% This also adds another test which makes sure that the sums over larger blocks work properly. I wrote this test when I was worried about memory corruption.

Patch Set 1 #

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

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

Total comments: 11

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

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+324 lines, -3 lines) Patch
M src/pkg/crypto/md5/gen.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/crypto/md5/md5_test.go View 1 1 chunk +24 lines, -0 lines 0 comments Download
M src/pkg/crypto/md5/md5block.go View 1 1 chunk +1 line, -1 line 0 comments Download
A src/pkg/crypto/md5/md5block_arm.s View 1 2 3 1 chunk +297 lines, -0 lines 1 comment Download
M src/pkg/crypto/md5/md5block_decl.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 20
Nick Craig-Wood
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
9 years ago (2013-07-21 07:55:18 UTC) #1
dfc
Protip, misc/benchcmp before.txt after.txt On 21/07/2013, at 17:55, nickcw@gmail.com wrote: > Reviewers: golang-dev1, > > ...
9 years ago (2013-07-21 08:00:10 UTC) #2
Nick Craig-Wood
On 2013/07/21 08:00:10, dfc wrote: > Protip, misc/benchcmp before.txt after.txt I've updated the description with ...
9 years ago (2013-07-21 08:21:55 UTC) #3
dfc
No worries, I'm delighted to have more people enthusiastic to work on arm. On 21/07/2013, ...
9 years ago (2013-07-21 08:23:10 UTC) #4
dfc
Very nicely done, very clear, thank you. Cc'ing remy and minux who more about the ...
9 years ago (2013-07-22 04:19:53 UTC) #5
dfc
Results from linux/arm on RPi raspberrypi(~/go/src/pkg/crypto/md5) % ~/go/misc/benchcmp {old,new}.txt benchmark old ns/op new ns/op delta ...
9 years ago (2013-07-22 04:20:39 UTC) #6
bradfitz
Next up: SHA-1? :-) Camlistore on Raspberry Pi would like that. On Sun, Jul 21, ...
9 years ago (2013-07-22 04:23:17 UTC) #7
dfc
linux/arm pandaboard benchmark old ns/op new ns/op delta BenchmarkHash8Bytes 4419 3333 -24.58% BenchmarkHash1K 15271 13449 ...
9 years ago (2013-07-22 12:27:06 UTC) #8
Nick Craig-Wood
https://codereview.appspot.com/11648043/diff/6001/src/pkg/crypto/md5/md5block_arm.s File src/pkg/crypto/md5/md5block_arm.s (right): https://codereview.appspot.com/11648043/diff/6001/src/pkg/crypto/md5/md5block_arm.s#newcode37 src/pkg/crypto/md5/md5block_arm.s:37: TEXT ·block(SB), 7, $(20*4)-16 On 2013/07/22 04:19:53, dfc wrote: ...
9 years ago (2013-07-22 15:37:40 UTC) #9
Nick Craig-Wood
9 years ago (2013-07-22 18:31:12 UTC) #10
Nick Craig-Wood
On 2013/07/22 12:27:06, dfc wrote: > linux/arm pandaboard Interesting - thanks for testing on other ...
9 years ago (2013-07-22 18:32:17 UTC) #11
rsc
https://codereview.appspot.com/11648043/diff/6001/src/pkg/crypto/md5/md5block_arm.s File src/pkg/crypto/md5/md5block_arm.s (right): https://codereview.appspot.com/11648043/diff/6001/src/pkg/crypto/md5/md5block_arm.s#newcode5 src/pkg/crypto/md5/md5block_arm.s:5: // ARM version of md5block.go by Nick Craig-Wood Delete. ...
9 years ago (2013-07-22 18:41:51 UTC) #12
dfc
On 2013/07/22 18:32:17, Nick Craig-Wood wrote: > On 2013/07/22 12:27:06, dfc wrote: > > linux/arm ...
9 years ago (2013-07-22 22:40:55 UTC) #13
ajstarks
Visualization of the Raspi and Pandaboard benchmarks: http://www.flickr.com/photos/ajstarks/9348936988/ On Monday, July 22, 2013 8:27:07 AM ...
9 years ago (2013-07-23 02:53:47 UTC) #14
Nick Craig-Wood
Thanks for the feedback - I'll fix those things and resubmit shortly. https://codereview.appspot.com/11648043/diff/6001/src/pkg/crypto/md5/md5block_arm.s File src/pkg/crypto/md5/md5block_arm.s ...
9 years ago (2013-07-23 08:25:13 UTC) #15
Nick Craig-Wood
I hope I've addressed everyone's concerns - please take another look! The changes since the ...
9 years ago (2013-07-23 22:17:56 UTC) #16
dfc
LGTM. Over to rsc for his sign off.
9 years ago (2013-07-24 10:49:03 UTC) #17
rsc
LGTM https://codereview.appspot.com/11648043/diff/25001/src/pkg/crypto/md5/md5block_arm.s File src/pkg/crypto/md5/md5block_arm.s (right): https://codereview.appspot.com/11648043/diff/25001/src/pkg/crypto/md5/md5block_arm.s#newcode297 src/pkg/crypto/md5/md5block_arm.s:297: GLOBL ·table(SB), $256 ,8,$256 please; that will make ...
9 years ago (2013-07-24 13:08:44 UTC) #18
dfc
This built fine on my arm5 host. If there are no objections I will address ...
9 years ago (2013-07-25 02:30:26 UTC) #19
dfc
9 years ago (2013-07-25 03:24:47 UTC) #20
*** Submitted as https://code.google.com/p/go/source/detail?r=40bbcafc819c ***

crypto/md5: native arm assembler version

An ARM version of md5block.go with a big improvement in
throughput (up to 2.5x) and a reduction in object size (21%).

Code size

  Before 3100 bytes
  After 2424 bytes
  21% smaller

Benchmarks on Rasperry Pi

benchmark                       old ns/op    new ns/op    delta
BenchmarkHash8Bytes                 11703         6636  -43.30%
BenchmarkHash1K                     38057        21881  -42.50%
BenchmarkHash8K                    208131       142735  -31.42%
BenchmarkHash8BytesUnaligned        11457         6570  -42.66%
BenchmarkHash1KUnaligned            69334        26841  -61.29%
BenchmarkHash8KUnaligned           455120       182223  -59.96%

benchmark                        old MB/s     new MB/s  speedup
BenchmarkHash8Bytes                  0.68         1.21    1.78x
BenchmarkHash1K                     26.91        46.80    1.74x
BenchmarkHash8K                     39.36        57.39    1.46x
BenchmarkHash8BytesUnaligned         0.70         1.22    1.74x
BenchmarkHash1KUnaligned            14.77        38.15    2.58x
BenchmarkHash8KUnaligned            18.00        44.96    2.50x

benchmark                      old allocs   new allocs    delta
BenchmarkHash8Bytes                     1            0  -100.00%
BenchmarkHash1K                         2            0  -100.00%
BenchmarkHash8K                         2            0  -100.00%
BenchmarkHash8BytesUnaligned            1            0  -100.00%
BenchmarkHash1KUnaligned                2            0  -100.00%
BenchmarkHash8KUnaligned                2            0  -100.00%

benchmark                       old bytes    new bytes    delta
BenchmarkHash8Bytes                    64            0  -100.00%
BenchmarkHash1K                       128            0  -100.00%
BenchmarkHash8K                       128            0  -100.00%
BenchmarkHash8BytesUnaligned           64            0  -100.00%
BenchmarkHash1KUnaligned              128            0  -100.00%
BenchmarkHash8KUnaligned              128            0  -100.00%

This also adds another test which makes sure that the sums
over larger blocks work properly. I wrote this test when I was
worried about memory corruption.

R=golang-dev, dave, bradfitz, rsc, ajstarks
CC=golang-dev, minux.ma, remyoudompheng
https://codereview.appspot.com/11648043

Committer: Dave Cheney <dave@cheney.net>
Sign in to reply to this message.

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