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

perf: fmt.Errorf used instead of errors.New #9749

Closed
egonelbre opened this issue Apr 11, 2024 · 3 comments · May be fixed by #10752
Closed

perf: fmt.Errorf used instead of errors.New #9749

egonelbre opened this issue Apr 11, 2024 · 3 comments · May be fixed by #10752
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@egonelbre
Copy link
Contributor

Throughout the whole codebase there are many places that use fmt.Errorf whereas errors.New can be used. Using fmt.Errorf is roughly 4x slower than errors.New.

This can be easily benchmarked with:

var result error

func BenchmarkFmtError(b *testing.B) {
	for range b.N {
		result = fmt.Errorf("something")
	}
}

func BenchmarkErrorsNew(b *testing.B) {
	for range b.N {
		result = errors.New("something")
	}
}

On my computer I'm getting:

BenchmarkFmtError-32             9535638               123.8 ns/op            32 B/op          2 allocs/op
BenchmarkErrorsNew-32           32106765                33.87 ns/op           16 B/op          1 allocs/op

This performance issue can be automatically fixed with:

gofmt -w -r "fmt.Errorf(s) -> errors.New(s)" .

Those error paths usually aren't hot paths, however it might impact the size of code generated.

@codyoss codyoss added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Apr 11, 2024
@codyoss
Copy link
Member

codyoss commented Apr 11, 2024

Thanks for the detailed report. This is a good suggestion. It looks like a good number of these are in our generated code so we can patch our generator to remove a lot of these and gofmt the rest.

codyoss added a commit to codyoss/gapic-generator-go that referenced this issue Apr 11, 2024
codyoss added a commit to googleapis/gapic-generator-go that referenced this issue Apr 11, 2024
@quartzmo
Copy link
Member

@codyoss Can this be closed now?

@codyoss codyoss closed this as completed Aug 12, 2024
@egonelbre
Copy link
Contributor Author

There are still 137 cases of fmt.Errorf being used in the repo. This can be easily seen by running gofmt -w -r "fmt.Errorf(s) -> errors.New(s)" on the whole repository.

egonelbre added a commit to egonelbre/google-cloud-go that referenced this issue Aug 22, 2024
Fixed automatically with:

  go fmt -w -r "fmt.Errorf(s) -> errors.New(s)" .
  goimports -w .

Updates googleapis#9749
gcf-merge-on-green bot pushed a commit that referenced this issue Sep 25, 2024
- perf(spanner): avoid using fmt.Errorf unnecessarily
- perf(spanner): avoid duplicated errors.New in UnmarshalJSON
- fix(spanner): error strings should not be capitalized

Updates #9749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants