code review 6850043: crypto/md5: add arm to md5block special case

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

[dave_cheney.net, 2012-11-13 23:55:22]

Hello golang-dev@googlegroups.com,

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

[r, 2012-11-14 00:06:03]

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.g...
src/pkg/crypto/md5/md5block.go:24: X = (*[16]uint32)(unsafe.Pointer(&p[0]))
can an ARM do unaligned fetches?

[dave_cheney.net, 2012-11-14 00:07:00]

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.g...
> src/pkg/crypto/md5/md5block.go:24: X = (*[16]uint32)(unsafe.Pointer(&p[0]))
> can an ARM do unaligned fetches?

I checked /proc/cpu/alignment and there were no unaligned accesses, but this was
an arm7 cpu. I will try on arm5

[minux1, 2012-11-14 11:44:20]

On Wed, Nov 14, 2012 at 8:07 AM, <dave@cheney.net> wrote:

> On 2012/11/14 00:06:03, r wrote:
>
>> can an ARM do unaligned fetches?
>>
>
> I checked /proc/cpu/alignment and there were no unaligned accesses, but
> this was an arm7 cpu. I will try on arm5
>
please apply this cl https://codereview.appspot.com/6782072/
and re-test on ARM.

[dave_cheney.net, 2012-11-15 01:04:25]

On 2012/11/14 11:44:20, minux wrote:
> On Wed, Nov 14, 2012 at 8:07 AM, <mailto:dave@cheney.net> wrote:
> 
> > On 2012/11/14 00:06:03, r wrote:
> >
> >> can an ARM do unaligned fetches?
> >>
> >
> > I checked /proc/cpu/alignment and there were no unaligned accesses, but
> > this was an arm7 cpu. I will try on arm5
> >
> please apply this cl https://codereview.appspot.com/6782072/
> and re-test on ARM.

 Curiously the test passes with your CL, but I was surprised it did.

[minux1, 2012-11-15 03:50:20]

On Thursday, November 15, 2012, wrote:

> On 2012/11/14 11:44:20, minux wrote:
>
>> On Wed, Nov 14, 2012 at 8:07 AM, <mailto:dave@cheney.net> wrote:
>>
>
>  > On 2012/11/14 00:06:03, r wrote:
>> >
>> >> can an ARM do unaligned fetches?
>> >>
>> >
>> > I checked /proc/cpu/alignment and there were no unaligned accesses,
>>
> but
>
>> > this was an arm7 cpu. I will try on arm5
>> >
>> please apply this cl
https://codereview.appspot.**com/6782072/<https://codereview.appspot.com/6782...
>> and re-test on ARM.
>>
>
>  Curiously the test passes with your CL, but I was surprised it did.
>
it depends on your /proc/cpu/alignment setting.
0 (ignore), 1(warn), 2 (fixup), 3 (fix+warn) will work as expected.

however, 4 (signal) won't.

[dave_cheney.net, 2012-11-15 04:19:19]

Yeah, i've been checking that, I can't run my pandaboard on setting 4
(the kernel wont' let me) but I see no alignment failures in user
mode.

On Thu, Nov 15, 2012 at 2:50 PM, minux <minux.ma@gmail.com> wrote:
>
> On Thursday, November 15, 2012, wrote:
>>
>> On 2012/11/14 11:44:20, minux wrote:
>>>
>>> On Wed, Nov 14, 2012 at 8:07 AM, <mailto:dave@cheney.net> wrote:
>>
>>
>>> > On 2012/11/14 00:06:03, r wrote:
>>> >
>>> >> can an ARM do unaligned fetches?
>>> >>
>>> >
>>> > I checked /proc/cpu/alignment and there were no unaligned accesses,
>>
>> but
>>>
>>> > this was an arm7 cpu. I will try on arm5
>>> >
>>> please apply this cl https://codereview.appspot.com/6782072/
>>> and re-test on ARM.
>>
>>
>>  Curiously the test passes with your CL, but I was surprised it did.
>
> it depends on your /proc/cpu/alignment setting.
> 0 (ignore), 1(warn), 2 (fixup), 3 (fix+warn) will work as expected.
>
> however, 4 (signal) won't.

[dave_cheney.net, 2012-11-15 07:26:10]

Thank you, your additional test fails on ARMv5.

=== RUN TestGolden
--- FAIL: TestGolden (0.00 seconds)
md5_test.go:73:         md5[3](For every action there is an equal and opposite
government program.) = c2df9afe383d86de9a7f165785085737 want
637d2fe925c07c113800509964fb0e06
=== RUN: ExampleNew
--- PASS: ExampleNew (0.00 seconds)

[dave_cheney.net, 2012-11-15 07:27:10]

*** Abandoned ***

[minux1, 2012-11-15 07:36:25]

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?

[dave_cheney.net, 2012-11-15 07:44:41]

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?