Skip to content

gh-125997: suggest efficient alternatives for time.sleep(0) #128752

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

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Jan 12, 2025

After a discussion with Victor and because #128274 is not convincing enough, we decide to:

  • keep the current behaviour of time.sleep(), and
  • document efficient (and probably semantically more correct) alternatives for time.sleep(0).

See #125997 (comment) for more details.


📚 Documentation preview 📚: https://cpython-previews--128752.org.readthedocs.build/

The implementation of `time.sleep()` changed in Python 3.11 and relies
on `clock_nanosleep()` or `nanosleep()` since then. This introduced a
regression in code using `time.sleep(0)` for a syscall "no-op", polling
or momentarily suspending the caller's thread.

To alleviate the performance regression, we suggest some alternatives
depending on the caller's needs.
@picnixz picnixz requested a review from vstinner January 12, 2025 18:48
@picnixz
Copy link
Member Author

picnixz commented Jan 12, 2025

@hauntsaninja and @charles-cooper: I would appreciate if you can share your thoughts on those suggestions as you were also involved in the other PR's discussion.

@charles-cooper
Copy link

i think it's interesting that select() is already mentioned as a possible implementation for time.sleep(), so maybe the approach in #128274 is valid actually.

https://github.com/python/cpython/pull/128752/files#diff-6203e0fc16084e67996c5bdd5e1caf4cc4699f67fde5da0abebe2ca239278ed4R402

besides that, i would just mention that clock_nanosleep may sleep at least 50us on linux. (i mean this is arguably a linux bug, but it is kind of surprising to users that sleep(0) sleeps a lot longer than one might expect).

@picnixz
Copy link
Member Author

picnixz commented Jan 12, 2025

sleep(0) sleeps a lot longer than one might expect

Actually, sleep() does not provide any guarantee that it will sleep exactly of the amount being requested. Granted it's surprising, but it's still documented in man nanosleep and others (sleep says "sleep() causes the calling thread to sleep either until the number of real-time seconds specified in seconds have elapsed" without saying "at least until" but I think it should be understood like this).

(), so maybe the approach in #128274 is valid actually.

While it's mentioned like that, I think it's wrong in the first place to rely on select() to implement sleep() (as I said on the issue). But since it's documented as a fallback, I think we should live with this discrepency, but we shouldn't make the "correct" implementation uses something else (namely "select()"). Even if the correct choice (clock_nanosleep/nanosleep) looks "buggy", that's what the caller should expect (I mean, I would expect time.sleep to rely on sleep-like functions).

Another solution is to check if we're on FreeBSD or not, but this makes maintainance harder. Also, for those reading the PR only, the sleep gap only happens at t = 0 and not at t = 1e-9 (for t = 1e-9, select() is still as slow as clock_nanosleep()). It's just that at t = 0, it returns immediately. So we can also just skip t = 0 calls on non-Windows platforms directly.

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this PR looks great, I think this is right approach.

I would not document current implementation choices of default Linux scheduler, especially since we've currently only had one report in >2 years. (If it does come up again, we can mention os.sched_setscheduler / PR_SET_TIMERSLACK)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM


To emulate a "no-op", use :keyword:`pass` instead of ``time.sleep(0)``.

To voluntarily relinquish the CPU, specify a real-time :ref:`scheduling
Copy link
Member

Choose a reason for hiding this comment

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

Instead of just repeating "relinquish the CPU" from sched_yield's docs, I think it's worth being more specific. Something like "allow other threads to execute" and/or "temporarily release the GIL" could be useful.

The other issue here is that sched_yield is specific to Unix systems--what are Windows users supposed to use if they want to explicitly let another thread take the GIL, if not time.sleep()?

Copy link
Member Author

Choose a reason for hiding this comment

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

They use time.sleep(0). The note is for the Unix section. On Windows, Sleep(0) relinquishes the CPU: https://learn.microsoft.com/en-gb/windows/win32/api/synchapi/nf-synchapi-sleep?redirectedfrom=MSDN.

A value of zero causes the thread to relinquish the remainder of its time slice to any other thread that is ready to run. If there are no other threads ready to run, the function returns immediately, and the thread continues execution. Windows XP: A value of zero causes the thread to relinquish the remainder of its time slice to any other thread of equal priority that is ready to run. If there are no other threads of equal priority ready to run, the function returns immediately, and the thread continues execution. This behavior changed starting with Windows Server 2003.

Copy link
Member Author

@picnixz picnixz Jan 13, 2025

Choose a reason for hiding this comment

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

Maybe I can add "On non-Windows platforms" if you want?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it. I didn't see that this was explicitly the Unix section, I thought you were just reformatting. (I still do think we should mention the GIL here, but it's up to you.)

Anyways, this is out of the scope of this PR, but this kind of situation probably isn't ideal for users. You have to maintain two code paths: one to call time.sleep(0) on Windows, and then os.sched_yield on Linux. Do you think it's a good idea to add a dedicated function for that in a follow-up issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. I don't know much about Windows users but maybe os.sched_yield() could, call Sleep(0) on Windows? Considering that no one ever complained about this specifically (what users complain is that it takes too much time, and probably don't really care about what we're actually doing behind the scene), I think we shouldn't bother for now.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably because it's difficult to notice--in practice, people use time.sleep(0) on both systems, because the performance issue on Linux generally isn't clear, especially for multithreaded programs. My theory is that providing a dedicated function will get people to notice the issue more. I'm speculating though!

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could open a DPO thread for that. Namely, whether to provide a Windows-equivalent of os.sched_yield() but that does not have the meaning of a scheduling policy.

Copy link
Member

Choose a reason for hiding this comment

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

IMO you're trying to solve a non-existent problem. time.sleep(0) is efficient and does what the developer expects. It's just that os.sched_yield() can be even more efficient on Linux. IMO time.sleep(0) is the portable "pass the CPU to another thread", even if it's not "optimal" (on Linux).

Copy link
Member

Choose a reason for hiding this comment

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

If you really need the most efficient "yield":

if hasattr(os, 'sched_yield'):
    def portable_efficient_yield():
        os.sched_yield()
else:
    def portable_efficient_yield():
        time.sleep(0)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I made a DPO thread, feel free to post concerns there: https://discuss.python.org/t/platform-agnostic-yielding-of-the-gil/77006/

I'm in favor of a function other than time.sleep(0) for yielding, because it's not particularly clear that sleep(0) would actually do something useful, but we're killing two birds with one stone because of the performance.

@picnixz
Copy link
Member Author

picnixz commented Jan 18, 2025

@vstinner I plan to merge this PR with no commit message and the current PR title. I don't want to expand more on the rationale in the commit and I think it's better to look at the issue directly. WDYT?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. Go ahead.

@picnixz picnixz merged commit f4afaa6 into python:main Jan 18, 2025
25 checks passed
@picnixz picnixz deleted the doc/time/sleep-125997 branch January 18, 2025 11:02
@picnixz picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 18, 2025
@miss-islington-app
Copy link

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 18, 2025
@miss-islington-app
Copy link

Sorry, @picnixz, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f4afaa6f1190fbbea3e27c590096951d8ffdfb71 3.12

@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

GH-128984 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 18, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

GH-128985 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jan 18, 2025
picnixz added a commit that referenced this pull request Jan 18, 2025
…H-128752) (#128984)

gh-125997: suggest efficient alternatives for `time.sleep(0)` (GH-128752)
(cherry picked from commit f4afaa6)

Co-authored-by: Bénédikt Tran <[email protected]>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants