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: performance regression in New #30468

Open
bcmills opened this Issue Feb 28, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@bcmills
Copy link
Member

bcmills commented Feb 28, 2019

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 #29382 (comment)).

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

This comment has been minimized.

Copy link
Member Author

bcmills commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member Author

bcmills commented Feb 28, 2019

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Contributor

neild commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Member

mvdan commented Feb 28, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Mar 13, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 13, 2019

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

gopherbot pushed a commit that referenced this issue Mar 14, 2019

errors: record only single frame
See Issue #29382 and Issue #30468.

3 frames are no longer needed as of
https://go-review.googlesource.com/c/go/+/152537/

name                     old time/op  new time/op  delta
New-8                     475ns ± 3%   352ns ± 2%  -25.87%  (p=0.008 n=5+5)
Errorf/no_format-8        661ns ± 4%   558ns ± 2%  -15.63%  (p=0.008 n=5+5)
Errorf/with_format-8      729ns ± 6%   626ns ± 2%  -14.23%  (p=0.008 n=5+5)
Errorf/method:_mytype-8  1.00µs ± 9%  0.84µs ± 2%  -15.94%  (p=0.008 n=5+5)
Errorf/method:_number-8  1.25µs ± 7%  1.04µs ± 2%  -16.38%  (p=0.008 n=5+5)

Change-Id: I30377e769b3b3be623f63ecbe365f8950ca08dda
Reviewed-on: https://go-review.googlesource.com/c/go/+/167400
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>

gopherbot pushed a commit that referenced this issue Mar 14, 2019

errors: improve performance of New
See Issue #29382 and Issue #30468.

Improvements in this CL:

name                     old time/op  new time/op  delta
New-8                     352ns ± 2%   225ns ± 5%  -36.04%  (p=0.008 n=5+5)

Improvements together with moving to 1 uintptr:

name                     old time/op  new time/op  delta
New-8                     475ns ± 3%   225ns ± 5%  -52.59%  (p=0.008 n=5+5)

Change-Id: I9d69a14e5e10a6498767defb7d5f26ceedcf9ba5
Reviewed-on: https://go-review.googlesource.com/c/go/+/167401
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.