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
net/http: HTTP/1 server got 0.5μs slower in Go 1.8 #18964
Comments
Too late for Go 1.8, but we can look into performance during Go 1.9. |
Anybody want to investigate the difference? What's a Go 1.7 vs Go 1.8 CPU profile say? |
Updated play with pprof: https://play.golang.org/p/GZ4zQOg1Wf I ran go tip
go 1.7.5
Let me know if you want me to do anything specific. |
Ouch, I'm seeing the same thing with about a 20% slowdown on my webserver benchmark with 1.8r3, and a slightly bigger binary size too |
I realize it's late on the release cycle but 20% regression on http is a huge regression for a lot of people. I'm willing to help debug if you tell me what's needed @bradfitz. |
Step 1 is debugging why it got slower. If you find any clues, let me know. |
@OneOfOne how does this work for non-hello world applications? Do you see any issues there? Maybe 20% for Hello World apps is acceptable until 1.9? |
I am not able to reproduce the OP's results. I'm on a Mac, and I'm using slightly older versions of Go.
There's a slight difference but it's smaller than 20%, almost negligible here. |
@kataras, do you have evidence in favor of that theory? You can confirm it yourself: delete those lines and measure again. I would be surprised, though. |
@kataras That looks like it is probably it. In addition to the select you have a mutex acquire and a unlock that is done in a defer (which I know recently got sped up, but are still a bit slower than a direct unlock. func (s *Server) getDoneChan() <-chan struct{} {
s.mu.Lock()
defer s.mu.Unlock()
return s.getDoneChanLocked()
} Since the accept loop is such a hot path, it may be worth moving the shutdown select into a separate goroutine and using sync/atomic to signal shutdown in the accept loop. EDIT: |
I understand that's too late for 1.8, but why not maybe fix this in a future 1.8.1 version? |
Has anyone else even been able to reproduce these results? |
-11.2% on Debian 8, amd64 (1.7.5 and rc3)
|
@kataras The implementation is correct, which means the author was careful.. it's not that big of a deal bugs happen in software. That said I was wrong anyways, I looked again just now and realized the select only happens when an error occurs during accept. What is odd is how it slowed down my program consistently when I remove it from the accept. Maybe it disqualifies that block from some optimization passes or inlining? There may be a problem somewhere causing a slowdown, but it's somewhere else and needs a different setup than mine. |
@codido, thanks for the bisect! Yes, and that's the commit I would suspect too. Good to see confirmation. That was one of the biggest architectural changes in the net/http.Server in quite some time. I never did any benchmarking (or optimizations) after that change. We can look into it for Go 1.9. |
Hi @bradfitz Just a quick question from an outsider, so please bear with me if I missed something obvious. If not, please educate me. Also, thanks for working on this project, I know a lot of people who love this language and the community around it. 🎉 |
Why are people responding and voting as if this is a major issue? This change seems to have a worst case performance impact of ~40us per request. That sounds very low to me, is there a real world scenario where this would matter? (edit: I was wrong, it's ~0.5us per request, so even lower) |
I think there might need to be more benchmarks around this to see the effect on things other than hello world. Things like hello world stick a lot of benchmark pressure on the internals and as such can magnify the effects of slowdowns. In a real world application, there's a boat-load more code that runs which in theory makes the effect of things like this far smaller per request - ie it becomes less of a % per request which means the effect is reduced. |
@reimertz I remember reading about the release cycle here: https://github.com/golang/go/wiki/Go-Release-Cycle
|
@kmlx thanks for the link! I only went here: https://golang.org/doc/contribute.html and searched for 'release' and couldn't find anything. My bad. Also, wow! If Google will run this version on their production servers and are okay with a worst case performance impact of ~40us per request (quoting @tinco ), I guess the rest of the world can as well. 🙂 |
This is why we ask people to test beta releases. So that if they believe they've found an issue, they can complain early. go1.8beta1 was released on 1, Dec 2016, more than 2 months ago: https://groups.google.com/d/topic/golang-nuts/QYuo0fai6YE/discussion Quoting the announcement:
|
Sorry I looked at the numbers wrong. So from @OneOfOne's test, |
@tinco I agree, it's a very minor regression when put into perspective. However, it's still very important to figure out why it regressed. Unless you want a situation like: #6853 and Go 1.9 comes around with another 11% decrease. That being said I'm not sure why this couldn't be fixed with a patch release(vs a minor release). |
@tinco , how many cores that computer has? multiply 0.5us by number of cores. |
On Wed, Feb 8, 2017 at 4:13 PM, Sokolov Yura ***@***.***> wrote:
how many cores that computer has? multiply 0.5us by number of cores.
Why does the number of cores has to do with this.
each request is only handled by a single core.
|
@minux the 5 million requests are handled in 30 seconds by N cores. So the real time spent per request on each core is 30 / 5M * N. N is likely rather small, less than 10 probably so it's not really relevant. |
Minor releases are for critical, unavoidable bugs. Your program has a 1% chance of randomly crashing after a day of execution is the kind of bug we fix in a point release, with the simplest, safest, most trivial fix possible. Minor releases are not for fixing "your program runs 0.5us slower to execute an operation that probably takes far longer than that overall anyway". |
I guess you're referring to patch releases ( |
In the Go project, we refer to 1.x as a major release and 1.x.y as a minor (or sometimes point) release: Go 1.8 is a major release; Go 1.8.2 is a minor or point release. It doesn't make sense for us to call Go 1.8 a minor release. The Go project release policy is documented at https://golang.org/doc/devel/release.html#policy:
Go 1.5 is backwards compatible with Go 1.4 with some caveats: your code has to not depend on undocumented behavior (for example, the ordering of equal elements chosen by sort.Sort), your code has to use keyed struct literals so it doesn't break if new struct fields are added, and so on. To the fullest extent possible, Go 1.5.1 is backwards compatible with Go 1.5 without any such caveats: we aim for updating from Go 1.5 to Go 1.5.1 to be a guaranteed-safe operation, a non-event, as you say "without changing features or any behavior". Accepting that making mistakes is an inevitable part of writing software and that any time you make a change there's a risk it will introduce a bug that isn't caught by testing before the release, the best way we know to reduce that risk is to disallow non-critical changes in point releases. Fixing a 0.5us slowdown that only appears in microbenchmarks is a non-critical change. |
@tinco if we don't consider this "so-called minor" performance issue and every time we ignore it, we'll ultimately get "a slower go". IMO no one wants this to happen. |
@rashidul0405 This issue is still open; no one is ignoring it. I was explaining why it doesn't merit a rushed fix into Go 1.8 or Go 1.8.1. I would gladly take a 1% slower Go instead of a 1% crashier Go. |
Let's keep discussion on mailing lists and Twitter etc. Please only comment here if you're working on this issue. |
Since no one appears to want to work on this, I'm going to close it. We can't chase down every possible performance detail, and this one is getting old. |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?Go 1.7.5, 1.8 rc3 and git
What operating system and processor architecture are you using (
go env
)?Arch Linux 64bit
What did you do?
go run
https://gist.github.com/OneOfOne/4d7e13977886ddab825870bc3422a901switch terminals and run
wrk -c 20 -d 30s http://localhost:8081
What did you expect to see?
Same throughput as 1.7 or faster.
What did you see instead?
go tip fd37b8c
go 1.8rc3 758a728
go 1.7.5
The text was updated successfully, but these errors were encountered: