-
-
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 19 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,9 @@ module time | |||||
|
|
||||||
| /* Forward declarations */ | ||||||
| static int pysleep(PyTime_t timeout); | ||||||
| #ifndef MS_WINDOWS | ||||||
| static int pysleep_zero_posix(void); // see gh-125997 | ||||||
| #endif | ||||||
|
|
||||||
|
|
||||||
| typedef struct { | ||||||
|
|
@@ -2213,6 +2216,12 @@ 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_posix(); | ||||||
| } | ||||||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| #endif | ||||||
|
|
||||||
| #ifndef MS_WINDOWS | ||||||
| #ifdef HAVE_CLOCK_NANOSLEEP | ||||||
|
|
@@ -2390,3 +2399,60 @@ pysleep(PyTime_t timeout) | |||||
| return -1; | ||||||
| #endif | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| #ifndef MS_WINDOWS | ||||||
| // 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_posix(void) | ||||||
picnixz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| { | ||||||
| assert(!PyErr_Occurred()); | ||||||
|
|
||||||
| 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.