code review 120930043: runtime: prevent pointless jmp in amd64 and 386 memmove

runtime: prevent pointless jmp in amd64 and 386 memmove

6a and 8a rearrange memmove such that the fallthrough from move_1or2 to move_0 ends up being a JMP to a RET. Insert an explicit RET to prevent such silliness.

Do the same for memclr as prophylaxis.

benchmark                old ns/op     new ns/op     delta
BenchmarkMemmove1        4.59          4.13          -10.02%
BenchmarkMemmove2        4.58          4.13          -9.83%

[josharian, 2014-07-30 04:31:27]

Hello golang-codereviews@googlegroups.com,

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

[dvyukov, 2014-07-30 05:18:43]

https://codereview.appspot.com/120930043/diff/40001/src/pkg/runtime/memclr_am...
File src/pkg/runtime/memclr_amd64.s (right):

https://codereview.appspot.com/120930043/diff/40001/src/pkg/runtime/memclr_am...
src/pkg/runtime/memclr_amd64.s:65: RET
I must be missing something, but the next instruction is RET anyway. What does
the additional RET change?

[minux, 2014-07-30 05:22:09]

I think it's more beneficial to teach liblink to
handle cases like this more ideal. (not for 1.4
though.)

Also, RET might not be a single instruction in
certain architectures, so duplicate it might not
always a win.

https://codereview.appspot.com/120930043/diff/40001/src/pkg/runtime/memclr_am...
File src/pkg/runtime/memclr_amd64.s (right):

https://codereview.appspot.com/120930043/diff/40001/src/pkg/runtime/memclr_am...
src/pkg/runtime/memclr_amd64.s:65: RET
On 2014/07/30 05:18:43, dvyukov wrote:
> I must be missing something, but the next instruction is RET anyway. What does
> the additional RET change?
the linker (liblink actually) is free to move basic blocks of code
around as it sees fit.

[ruiu, 2014-07-30 06:30:13]

On Tue, Jul 29, 2014 at 10:22 PM, <minux@golang.org> wrote:

> I think it's more beneficial to teach liblink to
> handle cases like this more ideal. (not for 1.4
> though.)
>
> Also, RET might not be a single instruction in
> certain architectures, so duplicate it might not
> always a win.


That is true, but because this change modifies the assembly files for x86
and amd64, I'd think you don't need to think about other architectures.


> https://codereview.appspot.com/120930043/diff/40001/src/
> pkg/runtime/memclr_amd64.s
> File src/pkg/runtime/memclr_amd64.s (right):
>
> https://codereview.appspot.com/120930043/diff/40001/src/
> pkg/runtime/memclr_amd64.s#newcode65
> src/pkg/runtime/memclr_amd64.s:65: RET
> On 2014/07/30 05:18:43, dvyukov wrote:
>
>> I must be missing something, but the next instruction is RET anyway.
>>
> What does
>
>> the additional RET change?
>>
> the linker (liblink actually) is free to move basic blocks of code
> around as it sees fit.
>
>
> https://codereview.appspot.com/120930043/
>
> --
> 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/d/optout.
>

[josharian, 2014-07-30 13:48:05]

>> I must be missing something, but the next instruction is RET anyway.
>>
> What does
>
>> the additional RET change?
>>
> the linker (liblink actually) is free to move basic blocks of code
> around as it sees fit.

To see this in action, run 'pkg/tool/darwin_amd64/6a -S
src/pkg/runtime/memmove_amd64.s'. move_1or2 is at 483. Observe that at 495,
instead of a RET as you'd expect from the source, you get JMP 143; 143 is
move_0.


> I think it's more beneficial to teach liblink to
> handle cases like this more ideal. (not for 1.4
> though.)

Sure, but this change is very simple and has immediate benefit, albeit small.
(Would you prefer I removed the memclr RET, since 6a/8a don't currently
rearrange that code?) Teaching liblink to trade off between alignment, extra
jumps, and extra instructions may not be simple and has uncertain benefit
relative to effort.

[bradfitz, 2014-07-31 17:39:16]

[+khr]



On Tue, Jul 29, 2014 at 9:31 PM, <josharian@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> runtime: prevent pointless jmp in amd64 and 386 memmove
>
> 6a and 8a rearrange memmove such that the fallthrough from move_1or2 to
> move_0 ends up being a JMP to a RET. Insert an explicit RET to prevent
> such silliness.
>
> Do the same for memclr as prophylaxis.
>
> benchmark                old ns/op     new ns/op     delta
> BenchmarkMemmove1        4.59          4.13          -10.02%
> BenchmarkMemmove2        4.58          4.13          -9.83%
>
> Please review this at https://codereview.appspot.com/120930043/
>
> Affected files (+7, -0 lines):
>   M src/pkg/runtime/memclr_386.s
>   M src/pkg/runtime/memclr_amd64.s
>   M src/pkg/runtime/memclr_plan9_386.s
>   M src/pkg/runtime/memmove_386.s
>   M src/pkg/runtime/memmove_amd64.s
>   M src/pkg/runtime/memmove_plan9_386.s
>   M src/pkg/runtime/memmove_plan9_amd64.s
>
>
> Index: src/pkg/runtime/memclr_386.s
> ===================================================================
> --- a/src/pkg/runtime/memclr_386.s
> +++ b/src/pkg/runtime/memclr_386.s
> @@ -63,6 +63,7 @@
>  clr_1or2:
>         MOVB    AX, (DI)
>         MOVB    AX, -1(DI)(BX*1)
> +       RET
>  clr_0:
>         RET
>  clr_3or4:
> Index: src/pkg/runtime/memclr_amd64.s
> ===================================================================
> --- a/src/pkg/runtime/memclr_amd64.s
> +++ b/src/pkg/runtime/memclr_amd64.s
> @@ -62,6 +62,7 @@
>  clr_1or2:
>         MOVB    AX, (DI)
>         MOVB    AX, -1(DI)(BX*1)
> +       RET
>  clr_0:
>         RET
>  clr_3or4:
> Index: src/pkg/runtime/memclr_plan9_386.s
> ===================================================================
> --- a/src/pkg/runtime/memclr_plan9_386.s
> +++ b/src/pkg/runtime/memclr_plan9_386.s
> @@ -32,6 +32,7 @@
>  clr_1or2:
>         MOVB    AX, (DI)
>         MOVB    AX, -1(DI)(BX*1)
> +       RET
>  clr_0:
>         RET
>  clr_3or4:
> Index: src/pkg/runtime/memmove_386.s
> ===================================================================
> --- a/src/pkg/runtime/memmove_386.s
> +++ b/src/pkg/runtime/memmove_386.s
> @@ -115,6 +115,7 @@
>         MOVB    -1(SI)(BX*1), CX
>         MOVB    AX, (DI)
>         MOVB    CX, -1(DI)(BX*1)
> +       RET
>  move_0:
>         RET
>  move_3or4:
> Index: src/pkg/runtime/memmove_amd64.s
> ===================================================================
> --- a/src/pkg/runtime/memmove_amd64.s
> +++ b/src/pkg/runtime/memmove_amd64.s
> @@ -122,6 +122,7 @@
>         MOVB    -1(SI)(BX*1), CX
>         MOVB    AX, (DI)
>         MOVB    CX, -1(DI)(BX*1)
> +       RET
>  move_0:
>         RET
>  move_3or4:
> Index: src/pkg/runtime/memmove_plan9_386.s
> ===================================================================
> --- a/src/pkg/runtime/memmove_plan9_386.s
> +++ b/src/pkg/runtime/memmove_plan9_386.s
> @@ -101,6 +101,7 @@
>         MOVB    -1(SI)(BX*1), CX
>         MOVB    AX, (DI)
>         MOVB    CX, -1(DI)(BX*1)
> +       RET
>  move_0:
>         RET
>  move_3or4:
> Index: src/pkg/runtime/memmove_plan9_amd64.s
> ===================================================================
> --- a/src/pkg/runtime/memmove_plan9_amd64.s
> +++ b/src/pkg/runtime/memmove_plan9_amd64.s
> @@ -104,6 +104,7 @@
>         MOVB    -1(SI)(BX*1), CX
>         MOVB    AX, (DI)
>         MOVB    CX, -1(DI)(BX*1)
> +       RET
>  move_0:
>         RET
>  move_3or4:
>
>
> --
> 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/d/optout.
>

[khr, 2014-07-31 19:30:10]

On 2014/07/31 17:39:16, bradfitz wrote:
> [+khr]
> 
> 
> 
> On Tue, Jul 29, 2014 at 9:31 PM, <mailto:josharian@gmail.com> wrote:
> 
> > Reviewers: golang-codereviews,
> >
> > Message:
> > Hello mailto:golang-codereviews@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> > runtime: prevent pointless jmp in amd64 and 386 memmove
> >
> > 6a and 8a rearrange memmove such that the fallthrough from move_1or2 to
> > move_0 ends up being a JMP to a RET. Insert an explicit RET to prevent
> > such silliness.
> >
> > Do the same for memclr as prophylaxis.
> >
> > benchmark                old ns/op     new ns/op     delta
> > BenchmarkMemmove1        4.59          4.13          -10.02%
> > BenchmarkMemmove2        4.58          4.13          -9.83%
> >
> > Please review this at https://codereview.appspot.com/120930043/
> >
> > Affected files (+7, -0 lines):
> >   M src/pkg/runtime/memclr_386.s
> >   M src/pkg/runtime/memclr_amd64.s
> >   M src/pkg/runtime/memclr_plan9_386.s
> >   M src/pkg/runtime/memmove_386.s
> >   M src/pkg/runtime/memmove_amd64.s
> >   M src/pkg/runtime/memmove_plan9_386.s
> >   M src/pkg/runtime/memmove_plan9_amd64.s
> >
> >
> > Index: src/pkg/runtime/memclr_386.s
> > ===================================================================
> > --- a/src/pkg/runtime/memclr_386.s
> > +++ b/src/pkg/runtime/memclr_386.s
> > @@ -63,6 +63,7 @@
> >  clr_1or2:
> >         MOVB    AX, (DI)
> >         MOVB    AX, -1(DI)(BX*1)
> > +       RET
> >  clr_0:
> >         RET
> >  clr_3or4:
> > Index: src/pkg/runtime/memclr_amd64.s
> > ===================================================================
> > --- a/src/pkg/runtime/memclr_amd64.s
> > +++ b/src/pkg/runtime/memclr_amd64.s
> > @@ -62,6 +62,7 @@
> >  clr_1or2:
> >         MOVB    AX, (DI)
> >         MOVB    AX, -1(DI)(BX*1)
> > +       RET
> >  clr_0:
> >         RET
> >  clr_3or4:
> > Index: src/pkg/runtime/memclr_plan9_386.s
> > ===================================================================
> > --- a/src/pkg/runtime/memclr_plan9_386.s
> > +++ b/src/pkg/runtime/memclr_plan9_386.s
> > @@ -32,6 +32,7 @@
> >  clr_1or2:
> >         MOVB    AX, (DI)
> >         MOVB    AX, -1(DI)(BX*1)
> > +       RET
> >  clr_0:
> >         RET
> >  clr_3or4:
> > Index: src/pkg/runtime/memmove_386.s
> > ===================================================================
> > --- a/src/pkg/runtime/memmove_386.s
> > +++ b/src/pkg/runtime/memmove_386.s
> > @@ -115,6 +115,7 @@
> >         MOVB    -1(SI)(BX*1), CX
> >         MOVB    AX, (DI)
> >         MOVB    CX, -1(DI)(BX*1)
> > +       RET
> >  move_0:
> >         RET
> >  move_3or4:
> > Index: src/pkg/runtime/memmove_amd64.s
> > ===================================================================
> > --- a/src/pkg/runtime/memmove_amd64.s
> > +++ b/src/pkg/runtime/memmove_amd64.s
> > @@ -122,6 +122,7 @@
> >         MOVB    -1(SI)(BX*1), CX
> >         MOVB    AX, (DI)
> >         MOVB    CX, -1(DI)(BX*1)
> > +       RET
> >  move_0:
> >         RET
> >  move_3or4:
> > Index: src/pkg/runtime/memmove_plan9_386.s
> > ===================================================================
> > --- a/src/pkg/runtime/memmove_plan9_386.s
> > +++ b/src/pkg/runtime/memmove_plan9_386.s
> > @@ -101,6 +101,7 @@
> >         MOVB    -1(SI)(BX*1), CX
> >         MOVB    AX, (DI)
> >         MOVB    CX, -1(DI)(BX*1)
> > +       RET
> >  move_0:
> >         RET
> >  move_3or4:
> > Index: src/pkg/runtime/memmove_plan9_amd64.s
> > ===================================================================
> > --- a/src/pkg/runtime/memmove_plan9_amd64.s
> > +++ b/src/pkg/runtime/memmove_plan9_amd64.s
> > @@ -104,6 +104,7 @@
> >         MOVB    -1(SI)(BX*1), CX
> >         MOVB    AX, (DI)
> >         MOVB    CX, -1(DI)(BX*1)
> > +       RET
> >  move_0:
> >         RET
> >  move_3or4:
> >
> >
> > --
> > 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 mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

LGTM.

Yes, it would be nice to fix liblink.  But this is OK for now.

[josharian, 2014-08-01 13:21:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=bfe2cc97b9e1 ***

runtime: prevent pointless jmp in amd64 and 386 memmove

6a and 8a rearrange memmove such that the fallthrough from move_1or2 to move_0
ends up being a JMP to a RET. Insert an explicit RET to prevent such silliness.

Do the same for memclr as prophylaxis.

benchmark                old ns/op     new ns/op     delta
BenchmarkMemmove1        4.59          4.13          -10.02%
BenchmarkMemmove2        4.58          4.13          -9.83%

LGTM=khr
R=golang-codereviews, dvyukov, minux, ruiu, bradfitz, khr
CC=golang-codereviews
https://codereview.appspot.com/120930043

[adonovan, 2014-08-04 22:05:19]

On 2014/08/01 13:21:15, josharian wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=bfe2cc97b9e1 ***
> 
> runtime: prevent pointless jmp in amd64 and 386 memmove
> 
> 6a and 8a rearrange memmove such that the fallthrough from move_1or2 to move_0
> ends up being a JMP to a RET. Insert an explicit RET to prevent such
silliness.
> 
> Do the same for memclr as prophylaxis.
> 
> benchmark                old ns/op     new ns/op     delta
> BenchmarkMemmove1        4.59          4.13          -10.02%
> BenchmarkMemmove2        4.58          4.13          -9.83%
> 
> LGTM=khr
> R=golang-codereviews, dvyukov, minux, ruiu, bradfitz, khr
> CC=golang-codereviews
> https://codereview.appspot.com/120930043

I think this CL may have caused a major performance regression.
The go.tools/go.loader test TestStdlib is running about 2x slower after this
specific CL.
I will continue to investigate...

[adonovan, 2014-08-04 22:17:06]

On 2014/08/04 22:05:19, adonovan wrote:
> On 2014/08/01 13:21:15, josharian wrote:
> > *** Submitted as https://code.google.com/p/go/source/detail?r=bfe2cc97b9e1
***
> > 
> > runtime: prevent pointless jmp in amd64 and 386 memmove
> > 
> > 6a and 8a rearrange memmove such that the fallthrough from move_1or2 to
move_0
> > ends up being a JMP to a RET. Insert an explicit RET to prevent such
> silliness.
> > 
> > Do the same for memclr as prophylaxis.
> > 
> > benchmark                old ns/op     new ns/op     delta
> > BenchmarkMemmove1        4.59          4.13          -10.02%
> > BenchmarkMemmove2        4.58          4.13          -9.83%
> > 
> > LGTM=khr
> > R=golang-codereviews, dvyukov, minux, ruiu, bradfitz, khr
> > CC=golang-codereviews
> > https://codereview.appspot.com/120930043
> 
> I think this CL may have caused a major performance regression.
> The go.tools/go.loader test TestStdlib is running about 2x slower after this
> specific CL.
> I will continue to investigate...

Sorry, false alarm.  Now it appears to be slower even at earlier CLs that were
previously not slow.  Must be some other cause.