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

context: optimize Err by removing defer #22401

Closed
dfawley opened this issue Oct 23, 2017 · 7 comments
Closed

context: optimize Err by removing defer #22401

dfawley opened this issue Oct 23, 2017 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Milestone

Comments

@dfawley
Copy link

dfawley commented Oct 23, 2017

go/src/context/context.go

Lines 335 to 339 in eca4599

func (c *cancelCtx) Err() error {
c.mu.Lock()
defer c.mu.Unlock()
return c.err
}

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

BenchmarkMutexWithDefer-12                	20000000	        51.2 ns/op
BenchmarkMutexWithoutDefer-12             	100000000	        16.5 ns/op

Also note the following benchmarks:
https://github.com/grpc/grpc-go/blob/master/benchmark/primitives/primitives_test.go#L29,L62

BenchmarkSelectClosed-12                  	50000000	        20.3 ns/op
BenchmarkSelectOpen-12                    	300000000	         5.74 ns/op

This ultimately means that the following code is more optimal in the case where an error is rare:

select {
case <-ctx.Done():
  return ctx.Err()
default:
}

vs.

if err := ctx.Err(); err != nil {
  return err
}

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.

@ianlancetaylor ianlancetaylor changed the title Optimize context.Err() by removing defer context: optimize Err by removing defer Oct 23, 2017
@ianlancetaylor
Copy link
Contributor

I would frankly rather fix #14939, at least for the unconditional single defer case, than complicate code across the standard library.

@dfawley
Copy link
Author

dfawley commented Oct 23, 2017

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.

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance labels Nov 1, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Nov 1, 2017
@ghost
Copy link

ghost commented Apr 15, 2018

Fixed with CL 107137.

@odeke-em
Copy link
Member

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.

@ghost
Copy link

ghost commented Apr 24, 2018

As not to be a credit thief, it was actually @keegancsmith's CL, not mine.

@odeke-em
Copy link
Member

Oops, thank you @keegancsmith for the CL :)

@keegancsmith
Copy link
Contributor

np

@golang golang locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
None yet
Development

No branches or pull requests

6 participants