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

*: use goroutine pool to avoid runtime.morestack #3753

Merged
merged 5 commits into from
Sep 22, 2017
Merged

Conversation

tiancaiamao
Copy link
Contributor

Maybe this branch works, you can have a try. @siddontang

@tiancaiamao tiancaiamao force-pushed the tiancaiamao/morestack branch from 23c762e to ae70f39 Compare July 18, 2017 06:11
@tiancaiamao tiancaiamao force-pushed the tiancaiamao/morestack branch from ae70f39 to a636621 Compare July 18, 2017 06:17
@hanfei1991 hanfei1991 added the DNM label Aug 21, 2017
@coocood
Copy link
Member

coocood commented Sep 12, 2017

@tiancaiamao
Please resolve the conflict

@tiancaiamao tiancaiamao changed the title [DNM] *: use goroutine pool to avoid runtime.morestack *: use goroutine pool to avoid runtime.morestack Sep 15, 2017
@tiancaiamao tiancaiamao changed the base branch from tiancaiamao/goroutine-pool to master September 15, 2017 07:07
@tiancaiamao tiancaiamao removed the DNM label Sep 18, 2017
@coocood
Copy link
Member

coocood commented Sep 21, 2017

@tiancaiamao
Can we remove the Skip in test now?

@@ -43,6 +44,8 @@ const (
minLogDuration = 50 * time.Millisecond
)

var xIndexSelectGP = gp.New(3 * time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

why use diffrent idleTimeout?

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 doesn't matter whether it 2mins or 3mins or anything.

reserveStack(false)
for _, batch1 := range batches {

batch := batch1
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use batch1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good habit ! @winkyao
When using closure along with range operation, it's easy to get suck:

	arr := []int{1, 2, 3, 4}
	for _, v := range arr {
		go func() {
			fmt.Println(v)   // guess the result?
		}()
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

got it!

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

Copy link
Member

@coocood coocood 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
Copy link
Member

coocood commented Sep 22, 2017

/run-all-test

@coocood coocood merged commit 040ad18 into master Sep 22, 2017
@coocood coocood deleted the tiancaiamao/morestack branch September 22, 2017 03:43
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.

4 participants