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

Issue 120930043: code review 120930043: runtime: prevent pointless jmp in amd64 and 386 memmove (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by josharian
Modified:
10 years, 7 months ago
Reviewers:
khr, adonovan
CC:
golang-codereviews, dvyukov, minux, ruiu, bradfitz, khr
Visibility:
Public.

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%

Patch Set 1 #

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

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

Total comments: 2

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -0 lines) Patch
M src/pkg/runtime/memclr_386.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memclr_amd64.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memclr_plan9_386.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memmove_386.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memmove_amd64.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memmove_plan9_386.s View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/runtime/memmove_plan9_amd64.s View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10
josharian
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
10 years, 7 months ago (2014-07-30 04:31:27 UTC) #1
dvyukov
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 I must be missing something, but the next ...
10 years, 7 months ago (2014-07-30 05:18:43 UTC) #2
minux
I think it's more beneficial to teach liblink to handle cases like this more ideal. ...
10 years, 7 months ago (2014-07-30 05:22:09 UTC) #3
ruiu
On Tue, Jul 29, 2014 at 10:22 PM, <minux@golang.org> wrote: > I think it's more ...
10 years, 7 months ago (2014-07-30 06:30:13 UTC) #4
josharian
>> I must be missing something, but the next instruction is RET anyway. >> > ...
10 years, 7 months ago (2014-07-30 13:48:05 UTC) #5
bradfitz
[+khr] On Tue, Jul 29, 2014 at 9:31 PM, <josharian@gmail.com> wrote: > Reviewers: golang-codereviews, > ...
10 years, 7 months ago (2014-07-31 17:39:16 UTC) #6
khr
On 2014/07/31 17:39:16, bradfitz wrote: > [+khr] > > > > On Tue, Jul 29, ...
10 years, 7 months ago (2014-07-31 19:30:10 UTC) #7
josharian
*** Submitted as https://code.google.com/p/go/source/detail?r=bfe2cc97b9e1 *** runtime: prevent pointless jmp in amd64 and 386 memmove 6a ...
10 years, 7 months ago (2014-08-01 13:21:15 UTC) #8
adonovan
On 2014/08/01 13:21:15, josharian wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=bfe2cc97b9e1 *** > > runtime: prevent ...
10 years, 7 months ago (2014-08-04 22:05:19 UTC) #9
adonovan
10 years, 7 months ago (2014-08-04 22:17:06 UTC) #10
Message was sent while issue was closed.
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.
Sign in to reply to this message.

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