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: consider removing osyield call from lock2 #69268

Open
rhysh opened this issue Sep 4, 2024 · 1 comment
Open

runtime: consider removing osyield call from lock2 #69268

rhysh opened this issue Sep 4, 2024 · 1 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
Milestone

Comments

@rhysh
Copy link
Contributor

rhysh commented Sep 4, 2024

The current runtime.lock2 implementation does a bit of spinning to try to acquire the runtime.mutex before sleeping. If a thread has gone to sleep within lock2 (via a syscall), it will eventually require another thread (in unlock2) to do another syscall to wake it. The bit of spinning allows us to avoid those syscalls in some cases. Slowing down a bit and trying again, at a high level, seems good; maybe the previous holder has exited the critical section.

The first phase of spinning involves a runtime.procyield call, which asks the processor to pause for a moment (on the scale of tens or hundreds of nanoseconds). There's some uncertainty about what that duration is and what it should be (described in part in #69232) but the idea of using this mechanism to slow down for a bit, again at a high level, seems good.

The second phase of spinning involves a runtime.osyield call. That's a syscall, implemented on Linux as a call to sched_yield(2). The discussion in CL 473656 links to https://www.realworldtech.com/forum/?threadid=189711&curpostid=189752 , which gives a perspective on why that's not a universally good idea.

  • It's a syscall, so it doesn't help with avoiding syscalls. (Though a single syscall here has a chance of avoiding a pair of syscalls, one to sleep indefinitely and one to wake).
  • The semantics aren't very well defined, and—very loosely speaking—don't align with our goals. We don't mean for the OS scheduler to drag a thread over from another NUMA node just because we said "we can't run at this instant".

Maybe we should delete that part of lock2. Or maybe we should replace it with an explicit nanosleep(2) call of some tiny time interval.

I don't see any urgency here. Mostly I'd like a tracking issue to reference in lock2's comments.

CC @golang/runtime

@rhysh rhysh added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 4, 2024
@gabyhelp
Copy link

gabyhelp commented Sep 4, 2024

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@mknyszek mknyszek modified the milestones: Backlog, Unplanned Sep 18, 2024
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

3 participants