Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: group all calls to runtime.panicindex #17371

Closed
rillig opened this issue Oct 6, 2016 · 3 comments
Closed

cmd/compile: group all calls to runtime.panicindex #17371

rillig opened this issue Oct 6, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.9Maybe

Comments

@rillig
Copy link
Contributor

rillig commented Oct 6, 2016

What version of Go are you using (go version)?

go version go1.7.1 windows/amd64

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\rillig\go
set GORACE=
set GOROOT=C:\Program Files (x86)\Go1.7.1
set GOTOOLDIR=C:\Program Files (x86)\Go1.7.1\pkg\tool\windows_amd64
set CGO_ENABLED=1

What did you do?

https://play.golang.org/p/vTaV_KfhNg

What did you expect to see?

All calls to runtime.panicindex and runtime.panicslice are grouped at the end of the function, so that they don't interrupt the control flow when a human reads the assembly code. This could also prevent some cache lines from being loaded, since the calls to panic are unlikely to be ever executed.

Alternatively, these panic calls should be near the code that could produce them, to make the JAE instruction shorter, using 2 bytes instead of 6.

What did you see instead?

The calls to panicindex and panicslice appear in the middle of the application code.

        varassign.go:82 0x401429        48898c2418010000                MOVQ CX, 0x118(SP)
        varassign.go:82 0x401431        488bac2498000000                MOVQ 0x98(SP), BP
        varassign.go:82 0x401439        4881c4a0000000                  ADDQ $0xa0, SP
        varassign.go:82 0x401440        c3                              RET
        varassign.go:6  0x401441        31c0                            XORL AX, AX
        varassign.go:82 0x401443        e94affffff                      JMP 0x401392
        varassign.go:81 0x401448        e843240200                      CALL runtime.panicslice(SB)
        varassign.go:81 0x40144d        0f0b                            UD2
        varassign.go:80 0x40144f        e83c240200                      CALL runtime.panicslice(SB)
        varassign.go:80 0x401454        0f0b                            UD2
        varassign.go:79 0x401456        e835240200                      CALL runtime.panicslice(SB)
        varassign.go:79 0x40145b        0f0b                            UD2
...
        varassign.go:17 0x4016ae        80fb61                          CMPL $0x61, BL
        varassign.go:22 0x4016b1        7209                            JB 0x4016bc
        varassign.go:17 0x4016b3        80fb7a                          CMPL $0x7a, BL
        varassign.go:22 0x4016b6        0f8611faffff                    JBE 0x4010cd

The RET instruction at 0x401440 even starts on a cache line on its own, and the rest of the cache line is probably unused.
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 9, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Oct 9, 2016
@bradfitz
Copy link
Contributor

As this is a performance thing, it seems unlikely for Go 1.8. @randall77, feel free to kick to Unplanned or Go1.9Maybe. (We're trying to use Unplanned a bit less than we used to)

@randall77
Copy link
Contributor

I'm not sure I agree with putting all the panics at the end. That might generate a marginal improvement in the number of instruction cache lines used, but it also might change jumps from 2 bytes to 6 bytes (on x86 at least), which could overwhelm any benefit.

Human readability isn't really a concern.

I feel like there is something we could do here, but it isn't clear to me exactly what that might be. And if we did something, how we would demonstrate any improvement (except making branches smaller, which should be easily measurable).

In any case, punting to 1.9.

@randall77 randall77 modified the milestones: Go1.9Maybe, Go1.8 Oct 18, 2016
@rillig
Copy link
Contributor Author

rillig commented Nov 15, 2016

I've changed my mind. Now I don't consider this "improvement" important enough to be implemented. For sure there are more interesting topics around.

@rillig rillig closed this as completed Nov 15, 2016
@golang golang locked and limited conversation to collaborators Nov 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
5 participants
@bradfitz @quentinmit @rillig @randall77 @gopherbot