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

util/goroutine_pool: add a goroutine pool package utilities #3752

Merged
merged 10 commits into from
Sep 12, 2017

Conversation

tiancaiamao
Copy link
Contributor

Reuse goroutine in a pool can avoid runtime.morestack. It may be useful sometimes.

BenchmarkGoPool-4          	 5000000	       392 ns/op
BenchmarkGo-4              	10000000	       161 ns/op
BenchmarkMorestackPool-4   	 2000000	       706 ns/op
BenchmarkMoreStack-4       	  500000	      3018 ns/op

Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

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

Please test this with sysbench.

@@ -0,0 +1,181 @@
// Copyright 2016 PingCAP, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

2016 -> 2017

@@ -0,0 +1,131 @@
// Copyright 2016 PingCAP, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

2016 -> 2017

@tiancaiamao
Copy link
Contributor Author

I've test in another branch, using pool mechanism can totally eliminate runtime.morestack.
PTAL @shenli @ngaut @winkyao

@zimulala zimulala added the DNM label Jul 19, 2017
ch: make(chan func()),
pool: pool,
}
go func(g *goroutine) {
Copy link
Member

Choose a reason for hiding this comment

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

I think defining a method for goroutine is more clear.

return ret
}

func (pool *Pool) put(p *goroutine) {
Copy link
Member

Choose a reason for hiding this comment

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

This is called by goroutine in background, I think make this a goroutine method is more reasonable.

}

func (pool *Pool) put(p *goroutine) {
p.next = nil
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it always nil, encured by get?


func (g *goroutine) put(pool *Pool) {
g.status = statusIdle
g.next = nil
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that g.next is not nil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure.


ret := head.next
head.next = ret.next
if ret == pool.tail {
Copy link
Member

Choose a reason for hiding this comment

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

pool.count--
if pool.count == 0 {
  pool.tail = head
}

is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't intent to maintain count, it's added later just for testing purpose.

}
// Status already changed from statusIdle => statusDying, delete this goroutine.
if atomic.LoadInt32(&g.status) == statusDying {
g.status = statusDead
Copy link
Member

Choose a reason for hiding this comment

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

It seems no body cares about this value any more.
It can be removed.

for {
g = pool.get()
if atomic.CompareAndSwapInt32(&g.status, statusIdle, statusInUse) {
break
Copy link
Member

Choose a reason for hiding this comment

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

I think just call
g.ch <-f and return here is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easier to read indeed, but there is a potential race condition.

}

func TestRace(t *testing.T) {
gp := New(200 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the timeout too long to test race?

@coocood
Copy link
Member

coocood commented Sep 9, 2017

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 11, 2017
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

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed DNM labels Sep 12, 2017
@tiancaiamao tiancaiamao merged commit 3245d49 into master Sep 12, 2017
@tiancaiamao tiancaiamao deleted the tiancaiamao/goroutine-pool branch September 19, 2017 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants