GeezerVison

@picnixz Unfortunately, we should seriously review :(. It seems that an assertion failed somewhere.

Assertion failed: (errno == 0), function pysleep, file timemodule.c, line 2220.
Fatal Python error: Aborted

Thread 0x0000000173bbb000 (most recent call first):
  File "/Users/admin/actions-runner/_work/cpython/cpython/Lib/subprocess.py", line 1918 in _execute_child
...

Ah I think I know what happens. When we raise an OSError from errno we don't clear the errno. So if you chain calls, you need to just ignore them. I'm removing the assertion cc @ZeroIntensity

If pytime is running on multiple cores, I recommend using _LIBC_REENTRANT, which will make your errno independent in each thread.

raw defined

#  if !defined _LIBC || defined _LIBC_REENTRANT
/* When using threads, errno is a per-thread value.  */
#   define errno (*__errno_location ())
#  endif
# endif /* !__ASSEMBLER__ */
#endif /* _ERRNO_H */

This wouldn't solve the issue (also because _LIBC_REENTRANT is a private macro and something you supply when configuring libc). By the way, errno is thread-safe (see https://linux.die.net/man/3/errno). The issue is simply that we can raise an OSError from errno, which doesn't clear errno, and do something else afterwards. I will keep the current behaviour as it was done beforehand.

TIL! Should we assert !PyErr_Occurred() instead?

TIL! Should we assert !PyErr_Occurred() instead?

Not sure. It's not really an issue and the function is internal and only for time.sleep(). It's only used once (it's just that time.sleep() calls this function). Though it wouldn't hurt (if this assertion fails, then there is a separate issue)

I think we should. There's no real cost to adding it, and it would be helpful for debugging in the rare case that something goes wrong.

I've locally selected the select() alternative, but considering your comment on the GIL, I may either release the GIL or drop this idea. What I can however keep from my PR (that I've still not updated) is the fact that emulating time.sleep(0) can be achieved using select.poll().poll(0) if one does not want to relinquish the CPU or via os.sched_yield() if one wants to relinquish the CPU.

i have not looked into this too deeply but my understanding is the poll() will also relinquish the CPU since it is a syscall but not necessarily give up the time slice (e.g. if there are no threads with higher priority)

In addition, I didn't check yet but I'm wondering whether calling clock_nanosleep() without calling PyTime_Monotonic would actually solve the issue of sleeping using a TIMER_ABSTIME flag. The reason is that clock_nanosleep() should not suspend the calling thread in this case:

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.

this looks like a good idea, might be the cleanest - just choose TIMER_ABSTIME in the case that the argument to time.sleep() is <=0.

as a user, i would just generally expect that sleep() issues a syscall (i.e. relinquish enough control to the kernel and cpython runtime that they can unblock any I/O or other operations that need to be scheduled) but don't really care exactly which one.

I'll commit something tomorrow or the day after using that approach. It would also be nice to avoid the (small) overhead of using PyTime_Monotonic().

While we're still above the select() alternative, we're now a bit faster and the delays are less pronounced: Mean +- std dev: 2.09 us +- 0.24 us.

For comparison, the current implementation performances 25x worse: Mean +- std dev: 54.8 us +- 0.8 us, so we're definitely seeing an improvement using the optimized version.

I'll update the comments, docs and tests and push those changes.

FTR, the numbers above are in a DEBUG build, but this doesn't change much as on an optimized build, current implementation takes roughly the same amount of time.

Ok, so I'm actually back at the starting point. First of all, let's take a step back and understand the issue at hand: time.sleep(0) takes too long when using clock_nanosleep or nanosleep. The reason? clock_nanosleep(0) takes roughly 1-2us; nanosleep(0) takes roughly 50us but select(0, NULL, NULL, NULL, &zero) takes 150ns.

Second, I was wrong with TIME_ABSTIMER combined with a 0 struct. The correct call would indeed be with flags=0 and {0, 0} OR with TIME_ABSTIMER and a timestamp value. Using TIME_ABSTIMER + 0 is the wrong call. Now, whether we use 0 or TIME_ABSTIMER with the correct timestamp, a single call to clock_nanosleep takes 50us. So, whatever choice we do, we'll always be slower.

Now, let's move back to what time.sleep(0) semantically does. What we want is to suspend the calling thread and release the GIL as well (otherwise it doesn't make sense). In C++, std::this_thread::sleep_for(0, 0) is implemented as follows (cleaning up the relevant bits):

template<typename _Rep, typename _Period> 
inline void
sleep_for(const chrono::duration<_Rep, _Period>& __rtime)
{
	if (__rtime <= __rtime.zero())
		return;
	...
}

So, as we can see, std::this_thread::sleep_for(std::chrono::seconds(0)) would essentially be a no-op. So in C++, time.sleep(0) would actually be a simple return. Do we want to emulate that behaviour?

If yes, what about Windows' dedicate sleep(0) which relinquishes the CPU? Should we also change it? I don't think so. Previously, non-Windows platforms implemented time.sleep using select. So why not switch back to that? or even better, align with C++ so that time.sleep(0) does nothing on Linux (maybe POSIX as well?)

Or, if this is really too much of a hassle, we can just... drop that PR (I won't mind) and simply improve the docs saying that time.sleep(0) may not be useful for polling and select.poll.poll(0) should be preferred.

For now, I've decided to revert to the old 3.11 behaviour. If you prefer me to change the docs, I can also do it. I'm leaving for a few days so I won't be able to see the comments.

If I understood correctly the issue, the purpose of this change is to make time.sleep(0) faster on Linux.

The problem is that always using select() to implement time.sleep(0) would make this call less efficient on FreeBSD: #125997 (comment)

Considering that we would be penalizing FreeBSD (and that using time.sleep(0) is usually not the correct semantic choice), I will close this PR and make two fresh PRs. One for adding tests and one for improving the documentation (namely, recommend using select.poll.poll(0) or os.sched_yield() depending on the caller needs; using time.sleep(0) is (in C++ at least) roughly equivalent to a pass (return immediately)).

I learned a lot during this but I'm still not comfortable with reverting it back to the previous behaviour. ISTM that the previous behaviour is probably "wrong" (namely, I'm not really sure we should even use select() as an alternative; we should probably use sleep() instead)