-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
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
Changes from 21 commits
3754e9e
3c0c4e6
c18f15f
8d270f2
2996937
80de853
c7aa428
f15436e
68e2988
afc6186
f6a817f
fb9eb41
ecb9636
1ed9877
23b4740
40155ff
153b326
7e351c9
285c54a
434da31
ff06516
40d56f1
c4ce9cc
54b6dde
405e473
b803045
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Ensure that :func:`time.sleep(0) <time.sleep>` does not degrade over time | ||
| on non-Windows platforms. Patch by Bénédikt Tran. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -72,6 +72,7 @@ module time | |||||
|
|
||||||
| /* Forward declarations */ | ||||||
| static int pysleep(PyTime_t timeout); | ||||||
| static int pysleep_zero(void); // see gh-125997 | ||||||
|
|
||||||
|
|
||||||
| typedef struct { | ||||||
|
|
@@ -2213,6 +2214,19 @@ static int | |||||
| pysleep(PyTime_t timeout) | ||||||
| { | ||||||
| assert(timeout >= 0); | ||||||
| assert(!PyErr_Occurred()); | ||||||
| #ifndef MS_WINDOWS | ||||||
| if (timeout == 0) { // gh-125997 | ||||||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| return pysleep_zero(); | ||||||
| } | ||||||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| #else | ||||||
| PyTime_t timeout_100ns = _PyTime_As100Nanoseconds(timeout, | ||||||
| _PyTime_ROUND_CEILING); | ||||||
| // Maintain Windows Sleep() semantics for time.sleep(0) | ||||||
| if (timeout_100ns == 0) { | ||||||
| return pysleep_zero(); | ||||||
| } | ||||||
| #endif | ||||||
|
|
||||||
| #ifndef MS_WINDOWS | ||||||
| #ifdef HAVE_CLOCK_NANOSLEEP | ||||||
|
|
@@ -2291,21 +2305,6 @@ 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; | ||||||
| } | ||||||
|
|
||||||
| LARGE_INTEGER relative_timeout; | ||||||
| // No need to check for integer overflow, both types are signed | ||||||
| assert(sizeof(relative_timeout) == sizeof(timeout_100ns)); | ||||||
|
|
@@ -2390,3 +2389,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. | ||||||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // | ||||||
| // Rationale | ||||||
| // --------- | ||||||
| // time.sleep(0) accumulates delays in the generic implementation, but we can | ||||||
picnixz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| // skip some calls to `PyTime_Monotonic()` and other checks when the timeout | ||||||
| // is zero. 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); | ||||||
|
||||||
| ret = clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &zero, NULL); | |
| ret = clock_nanosleep(CLOCK_MONOTONIC, 0, &zero, NULL); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
Outdated
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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:
select()is implementation sensitive but there were some semantical and implementation concerns (see gh-125997: ensure thattime.sleep(0)is not delayed on non-Windows platforms #128274 (comment)).- using
select()would be much faster and if you still want to go that road (which I did originally), then I personally don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that you're referring to performance:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the resolution of this PR (namely, whether we eventually use
select()), I'll amend the NEWS so I won't commit that suggestion for now.