Go: errors: performance regression in New

[bcmills]

CL 163557 added a call to runtime.Callers in the path of errors.New, which has now become a bottleneck in cmd/go initialization (see https://github.com/golang/go/issues/29382#issuecomment-468206700).

While I understand the desire to capture stack information in errors (#29934), errors.New in particular used to be an inexpensive operation, and it needs to remain inexpensive. Ideally, errors.New(someConstant) should be fully inlined: the caller PC is known at link-time, and that's all we should need in order to produce a reasonable error frame.

[bcmills]

Marking as release-blocker for 1.13: at the very least, we should investigate the impact of this regression in other low-latency programs and make an explicit decision about whether the regression is an acceptable cost.

[bcmills]

CC @mpvl @jba @neild @mvdan

[mvdan]

Thanks for filing this issue. I actually think runtime.Callers is reasonable if the errors.New happens outside of a global var declaration. But it's even better if the cost is always minimal.

[neild]

If it proves infeasible to make it sufficiently fast, dropping stack frames from errors.New and only adding them in fmt.Errorf could be an option. We should first see if the cost can be made minimal, though.

[mvdan]

I'm not sure that making errors.New do less work is ideal; it could lead to premature optimizations like "avoid fmt.Errorf for the sake of performance", even if the uses are outside of global variables.

It's also possibly reasonable to use fmt.Errorf for globals or in init, I'd imagine.

[gopherbot]

Change https://golang.org/cl/167401 mentions this issue: errors: improve performance of New

[gopherbot]

Change https://golang.org/cl/167400 mentions this issue: errors: record only single frame

[dmitshur]

This was fixed via CL 176997, which reverted the change that was causing this issue. /cc @andybons