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

*: remove goroutine pool #7564

Merged
merged 2 commits into from
Aug 31, 2018
Merged

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Benchmark on Go1.10:

➜ goroutine_pool git:(master) ✗ go test -test.bench Benchmark
goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/util/goroutine_pool
BenchmarkGoPool-4 5000000 382 ns/op
BenchmarkGo-4 10000000 163 ns/op
BenchmarkMorestackPool-4 1000000 1092 ns/op
BenchmarkMoreStack-4 500000 3012 ns/op
PASS
ok github.com/pingcap/tidb/util/goroutine_pool 8.303s

Benchmark on Go1.11:

➜ goroutine_pool git:(master) ✗ go test -test.bench Benchmark
goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/util/goroutine_pool
BenchmarkGoPool-4 5000000 326 ns/op
BenchmarkGo-4 10000000 176 ns/op
BenchmarkMorestackPool-4 2000000 719 ns/op
BenchmarkMoreStack-4 5000000 391 ns/op
PASS
ok github.com/pingcap/tidb/util/goroutine_pool 10.035s

We can see that BenchmarkMoreStack has improved dramatically, it's even faster than a pooled version.
So it's time to remove the goroutine pool.

What is changed and how it works?

goroutine pool was introduced to handle stack copy cost #3752 #3753

Go1.11 has many optimizations for stack copy:

https://go-review.googlesource.com/c/go/+/94029 runtime: speed up stack copying a little
https://go-review.googlesource.com/c/go/+/104737 runtime: avoid calling adjustpointers unnecessarily
https://go-review.googlesource.com/c/go/+/108945 runtime: add fast version of getArgInfo
https://go-review.googlesource.com/c/go/+/109716 runtime: iterate over set bits in adjustpointers
https://go-review.googlesource.com/c/go/+/109001 runtime: allow inlining of stackmapdata
https://go-review.googlesource.com/c/go/+/104175 cmd/compile: shrink liveness maps
These optimizations collectively improve stack copy performance by ~2X

After upgrading to Go1.1, goroutine pool is not necessary any more.

Check List

Tests

  • No code

Code changes

remove goroutine_pool package and update the code

goroutine pool was introduced to handle stack copy cost, Go1.11 many
optimizations for stack copy, after upgrading to Go1.1, goroutine
pool is not necessary any more.
@ngaut
Copy link
Member

ngaut commented Aug 31, 2018

LGTM

@winkyao
Copy link
Contributor

winkyao commented Aug 31, 2018

Cool.

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao tiancaiamao merged commit 2b776ac into pingcap:master Aug 31, 2018
@tiancaiamao tiancaiamao deleted the remove-goroutine-pool branch August 31, 2018 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants