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

errors: performance regression in New #30468

Closed
bcmills opened this issue Feb 28, 2019 · 8 comments
Closed

errors: performance regression in New #30468

bcmills opened this issue Feb 28, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance release-blocker
Milestone
Go1.13

Comments

@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2019 •
edited

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 bcmills added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 28, 2019
@bcmills bcmills added this to the Go1.13 milestone Feb 28, 2019
@bcmills
Copy link
Contributor 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
Copy link
Contributor Author

bcmills commented Feb 28, 2019

CC @mpvl @jba @neild @mvdan

@bcmills bcmills mentioned this issue Feb 28, 2019
Closed
@mvdan
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
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
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
Copy link

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

@gopherbot
Copy link

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

@mvdan mvdan mentioned this issue Mar 14, 2019
Closed
gopherbot pushed a commit that referenced this issue Mar 14, 2019
@mpvl
errors: record only single frame
5402854 
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
@mpvl
errors: improve performance of New
8bf18b5 
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>
@changkun changkun mentioned this issue Apr 17, 2019
Closed
@dmitshur
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@neild @dmitshur @mvdan @bcmills @gopherbot