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
context: optimize Err by removing defer #22401
Comments
I would frankly rather fix #14939, at least for the unconditional single defer case, than complicate code across the standard library. |
So would I, but #14939 has been open for 1.5 years, and this change would only take a couple minutes to make, so it may be worth doing as a temporary workaround. |
Fixed with CL 107137. |
Thanks for the fix @bontibon! @dfawley despite #14939 not having been fixed, your original request has been implemented by @bontibon's CL https://go-review.googlesource.com/c/go/+/107137 and commit 0b9c1ad. I'll close this issue, thank you @dfawley for the report/request, it'll def make grpc-go faster! Please reopen if needed. |
As not to be a credit thief, it was actually @keegancsmith's CL, not mine. |
Oops, thank you @keegancsmith for the CL :) |
np |
go/src/context/context.go
Lines 335 to 339 in eca4599
Defers currently result in lower performance vs. inline calls (#14939). See the following benchmarks for reference:
https://github.com/grpc/grpc-go/blob/master/benchmark/primitives/primitives_test.go#L140
https://github.com/grpc/grpc-go/blob/master/benchmark/primitives/primitives_test.go#L174
Also note the following benchmarks:
https://github.com/grpc/grpc-go/blob/master/benchmark/primitives/primitives_test.go#L29,L62
This ultimately means that the following code is more optimal in the case where an error is rare:
vs.
Unless there are plans to fix #14939 soon (which would be ideal), a trivial refactor to remove the defer will result in a ~3x speedup. If this is done, it might also be interesting to take a one-time pass through everything in the standard packages and remove other trivial defers encountered.
The text was updated successfully, but these errors were encountered: