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

errors: make errors.Trace mid-stack inlineable #12

Merged
merged 1 commit into from
Mar 18, 2019
Merged

errors: make errors.Trace mid-stack inlineable #12

merged 1 commit into from
Mar 18, 2019

Conversation

lysu
Copy link
Collaborator

@lysu lysu commented Mar 18, 2019

go 1.12 with more aggressive inline strategy

https://tip.golang.org/doc/go1.12

More functions are now eligible for inlining by default, including functions that do nothing but call another function.

so error.Trace can be inlined in 1.12, let nil check to Trace make less low level method calling

goos: linux
goarch: amd64
pkg: github.com/pingcap/errors
BenchmarkOld-8   	100000000	        18.3 ns/op
BenchmarkNew-8   	1000000000	         2.26 ns/op
PASS

with default -l, go1.12 also can inline Trace,

go build -gcflags '-m=2 -N' 
# github.com/pingcap/errors/m/x
./m.go:14:6: can inline f2 as: func() error { return errors.New("") }
./m.go:15:19: inlining call to errors.New func(string) error { return error(&errors.fundamental literal) }
./m.go:15:19: inlining call to errors.callers func() *errors.stack { return errors.callersSkip(int(4)) }
./m.go:10:6: cannot inline f1: function too complex: cost 146 exceeds budget 80
./m.go:11:25: inlining call to f2 func() error { return errors.New("") }
./m.go:11:25: inlining call to errors.New func(string) error { return error(&errors.fundamental literal) }
./m.go:11:25: inlining call to errors.callers func() *errors.stack { return errors.callersSkip(int(4)) }
./m.go:11:22: inlining call to errors.Trace2 func(error) error { if errors.err == nil { return nil }; return errors.AddStack(errors.err) }

on another side in go1.11.3

go build -gcflags '-m=2 -l=3 -N'  
# github.com/pingcap/errors/m/x
./m.go:14:6: cannot inline f2: function too complex: cost 84 exceeds budget 80
./m.go:10:6: cannot inline f1: function too complex: cost 165 exceeds budget 80
./m.go:5:6: cannot inline main: function too complex: cost 89 exceeds budget 80

and need -l=4 to make mid-stack inline, so this isn't work in version < 1.12

this modification is very similar to https://go-review.googlesource.com/c/go/+/156362/

This change is Reviewable

@gregwebs
Copy link

What happens if the nil check is placed in AddStack?

@lysu
Copy link
Collaborator Author

lysu commented Mar 18, 2019

@gregwebs I have do some test, it seems AddStack can not be inline

package main

import "github.com/pingcap/errors"

func main() {
	e := f1()
	e = e
}

func f1() error {
	return errors.AddStack2(nil)
}
go build -gcflags '-m=2 -N' 
# github.com/pingcap/errors/m/x
./m.go:10:6: can inline f1 as: func() error { return errors.AddStack2(nil) }
./m.go:5:6: can inline main as: func() { e := f1(); e = e }
./m.go:6:9: inlining call to f1 func() error { return errors.AddStack2(nil) }

so, it need jump into the method to check if-condition, so little slow 0.3 ns/op

@lysu lysu requested a review from coocood March 18, 2019 09:07
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ngaut ngaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants