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

runtime: make injectglist starting spinning m #69621

Open
jayantxie opened this issue Sep 25, 2024 · 7 comments
Open

runtime: make injectglist starting spinning m #69621

jayantxie opened this issue Sep 25, 2024 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@jayantxie
Copy link

jayantxie commented Sep 25, 2024

Proposal Details

Can we set the parameter spinning of the runtime.startm function be set to true by default to make full use of spinning ms to run tasks and avoid additional thread wake up, such as runtime.injectglist and runtime.handoffp?

@gopherbot gopherbot added this to the Proposal milestone Sep 25, 2024
@seankhliao seankhliao changed the title proposal: src/runtime: make injectglist starting spinning m? runtime: make injectglist starting spinning m Sep 25, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 25, 2024
@seankhliao seankhliao removed this from the Proposal milestone Sep 25, 2024
@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Sep 25, 2024
@ianlancetaylor
Copy link
Contributor

CC @golang/runtime

@mknyszek
Copy link
Contributor

This is a change that may be possible to make, but is not that simple to do correctly. Passing spinning=true can only happen if nmspinning is incremented, but just incrementing nmspinning in all of the places startm is called is probably wrong.

What are you concretely trying to achieve? Do you have a benchmark or application that you think would benefit? Why would it benefit? Without this information, I don't think we'll be able to move forward here. You could also try implementing it yourself and showing that it is an improvement on some benchmark or application, that would also help this issue move forward. Thanks.

@mknyszek mknyszek added this to the Unplanned milestone Sep 25, 2024
@jayantxie
Copy link
Author

jayantxie commented Sep 26, 2024

@mknyszek I'm not quite sure why runtime.wakep function sets spinning to true by default, while other cases such as runtime.injectglist do not. I have a conjecture that if the existence of spinning ms in Go code is very common, can we also change cases like runtime.injectglist to spinning, so as to make better use of these spinning ms and reduce unnecessary wakeup.

I tried to simulate such a scenario: a Go program receives n tasks through the network at regular intervals. At the same time, each task starts two goroutines to run. In this case, we have a large number of spinning ms created by runtime.newproc function, and due to the relatively large number of network tasks, netpoll will also call runtime.injectglist to create additional non-spinning threads. In this case, these non-spinning threads cannot fully utilize spinning threads to avoid starting, thus increasing additional overhead.

The following is the code to simulate this scenario:
https://github.com/jayantxie/spinningm

Run command: ./run.sh, and then modify $GOROOT/src/runtime/proc.go:

func injectglist(glist *gList) {
	// ...
	startIdle := func(n int) {
		for i := 0; i < n; i++ {
			wakep()
		}
	}
	// ...
}

run ./run.sh again.

And the result is,

  • injectglist without spinning:
image image
  • injectglist with spinning:
image image

Although this is only a special case and setting "spinning" by default may consume additional CPU when the system is very idle, we can still carefully evaluate this change because spinning calls like runtime.wakep in Go are very common.

@jayantxie
Copy link
Author

@mknyszek Is there any advice?

@mknyszek
Copy link
Contributor

Interesting, I see what you're saying. Off the top of my head, I don't see why wakep and being conservative about spinning Ms would be a problem here, and may be worth trying. It's also indeed a bit odd that startIdle actually looks so much like wakep. Though, it seems like the intent of this code is to get enough threads running to handle everything that just came out of the netpoller, and wakep is far more conservative than that: if there are any spinning Ms, it will back out. Taking this a step further, perhaps this should only start up max(npidle, n - nmspinning) threads (wakep that many times).

My concern, however, is that without an example of a real-world application dealing with this, that we'll just be making a change for the sake of making a change, without a strong sense of how it'll help Go users. Have you seen this happen in a production environment?

CC @prattmic

@mknyszek
Copy link
Contributor

#54622 is also related to the CPU cost of extraneous Ms spinning for work.

@jayantxie
Copy link
Author

jayantxie commented Oct 23, 2024

@mknyszek I found this problem in a self-developed network library. In the library I developed, I started a thread by newm but without a P. It will call a special network poller in a loop to obtain events , and then call injectglist to wake up the goroutines associated with these events. After testing, I found that if it is changed to spinning=true, the CPU usage under a fixed QPS can be significantly reduced. Therefore, I simulated a similar scenario with both injectglist and newproc calls to indirectly verify this phenomenon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Development

No branches or pull requests

5 participants