Skip to content

gh-125997: ensure that time.sleep(0) is not delayed on non-Windows platforms #128274

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

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Doc/whatsnew/3.14.rst
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,16 @@ sys.monitoring
Two new events are added: :monitoring-event:`BRANCH_LEFT` and
:monitoring-event:`BRANCH_RIGHT`. The ``BRANCH`` event is deprecated.


time
----

* Ensure that the duration of :func:`time.sleep(0) <time.sleep>` is as small
as possible on non-Windows platforms when :manpage:`clock_nanosleep(2)`
or :manpage:`nanosleep(2)` are used to implement :func:`!time.sleep`.
(Contributed by Bénédikt Tran in :gh:`125997`.)


tkinter
-------

Expand Down
13 changes: 11 additions & 2 deletions Lib/test/test_time.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,19 @@ def test_conversions(self):
self.assertEqual(int(time.mktime(time.localtime(self.t))),
int(self.t))

def test_sleep(self):
def test_sleep_exceptions(self):
self.assertRaises(TypeError, time.sleep, [])
self.assertRaises(TypeError, time.sleep, "a")
self.assertRaises(TypeError, time.sleep, complex(0, 0))

self.assertRaises(ValueError, time.sleep, -2)
self.assertRaises(ValueError, time.sleep, -1)
time.sleep(1.2)
self.assertRaises(ValueError, time.sleep, -0.1)

def test_sleep(self):
for value in [-0.0, 0, 0.0, 1e-6, 1, 1.2]:
with self.subTest(value=value):
time.sleep(value)

def test_epoch(self):
# bpo-43869: Make sure that Python use the same Epoch on all platforms:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Ensure that the duration of :func:`time.sleep(0) <time.sleep>` is as small
as possible on non-Windows platforms when :manpage:`clock_nanosleep(2)`
or :manpage:`nanosleep(2)` are used to implement :func:`!time.sleep`.
Patch by Bénédikt Tran.
86 changes: 71 additions & 15 deletions Modules/timemodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ module time

/* Forward declarations */
static int pysleep(PyTime_t timeout);
static int pysleep_zero(void); // see gh-125997


typedef struct {
Expand Down Expand Up @@ -2213,7 +2214,10 @@ static int
pysleep(PyTime_t timeout)
{
assert(timeout >= 0);

assert(!PyErr_Occurred());
if (timeout == 0) { // gh-125997
return pysleep_zero();
}
#ifndef MS_WINDOWS
#ifdef HAVE_CLOCK_NANOSLEEP
struct timespec timeout_abs;
Expand Down Expand Up @@ -2292,20 +2296,8 @@ pysleep(PyTime_t timeout)
return 0;
#else // MS_WINDOWS
PyTime_t timeout_100ns = _PyTime_As100Nanoseconds(timeout,
_PyTime_ROUND_CEILING);

// Maintain Windows Sleep() semantics for time.sleep(0)
if (timeout_100ns == 0) {
Py_BEGIN_ALLOW_THREADS
// 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.
Sleep(0);
Py_END_ALLOW_THREADS
return 0;
}

_PyTime_ROUND_CEILING);
assert(timeout_100ns > 0);
LARGE_INTEGER relative_timeout;
// No need to check for integer overflow, both types are signed
assert(sizeof(relative_timeout) == sizeof(timeout_100ns));
Expand Down Expand Up @@ -2390,3 +2382,67 @@ pysleep(PyTime_t timeout)
return -1;
#endif
}


// time.sleep(0) optimized implementation.
// On error, raise an exception and return -1.
// On success, return 0.
//
// Rationale
// ---------
// time.sleep(0) is slower when using the generic implementation, but we make
// it faster than time.sleep(eps) for eps > 0 so to avoid some performance
// annoyance. For details, see https://github.com/python/cpython/pull/128274.
static int
pysleep_zero(void)
{
assert(!PyErr_Occurred());
#ifndef MS_WINDOWS
int ret, err;
Py_BEGIN_ALLOW_THREADS
#ifdef HAVE_CLOCK_NANOSLEEP
struct timespec zero = {0, 0};
ret = clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &zero, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that TIMER_ABSTIME is appropriate here:

Suggested change
ret = clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &zero, NULL);
ret = clock_nanosleep(CLOCK_MONOTONIC, 0, &zero, NULL);

Copy link
Member Author

@picnixz picnixz Jan 7, 2025

Choose a reason for hiding this comment

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

I first thought about:

If flags is TIMER_ABSTIME, then t is interpreted as an absolute
time as measured by the clock, clockid. If t is less than or
equal to the current value of the clock, then clock_nanosleep()
returns immediately without suspending the calling thread.

Without this flag, time.sleep(0) takes 50us. Otherwise it takes 2us. So, now I'm more and more inclined to actually revert it back to a select. Because otherwise, it's as if I'm not doing anything (and just skip the call to clock_nanosleep; I mean I already know that the condition on t is verified so it's as if I have no call)

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'm leaving for a few days but I'm still struggling to convince myself between the use of select or not. The reason why clock_nanosleep() is slowed down is because we don't pass a zero time struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I was wrong. See #128274 (comment).

err = ret;
#elif defined(HAVE_NANOSLEEP)
struct timespec zero = {0, 0};
ret = nanosleep(&zero, NULL);
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 worth it to have 3 code paths for sleep(0)? Can't we always use select()?

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a long discussion on this matter but long story short:

err = errno;
#else
// POSIX-compliant select(2) allows the 'timeout' parameter to
// be modified but also mandates that the function should return
// immediately if *both* structure's fields are zero (which is
// the case here).
//
// However, since System V (but not BSD) variant typically sets
// the timeout before returning (but does not specify whether
// this is also the case for zero timeouts), we prefer supplying
// a fresh timeout everytime.
struct timeval zero = {0, 0};
ret = select(0, NULL, NULL, NULL, &zero);
err = errno;
#endif
Py_END_ALLOW_THREADS
if (ret == 0) {
return 0;
}
if (err != EINTR) {
errno = err;
PyErr_SetFromErrno(PyExc_OSError);
return -1;
}
/* sleep was interrupted by SIGINT */
if (PyErr_CheckSignals()) {
return -1;
}
#else // Windows implementation
Py_BEGIN_ALLOW_THREADS
// 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.
Sleep(0);
Py_END_ALLOW_THREADS
#endif
return 0;
}
Loading