gh-133465: Allow PyErr_CheckSignals to be called without holding the GIL. #133466
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| :c:func:`PyErr_CheckSignals` has been changed to acquire the global | ||
| interpreter lock (GIL) itself, only when necessary (i.e. when it has work to | ||
| do). This means that modules that perform lengthy computations with the GIL | ||
| released may now call :c:func:`PyErr_CheckSignals` during those computations | ||
| without re-acquiring the GIL first. (However, it must be *safe to* acquire | ||
| the GIL at each point where :c:func:`PyErr_CheckSignals` is called. Also, | ||
| keep in mind that it can run arbitrary Python code before returning to you.) |
There was a problem hiding this comment.
A NEWS entry should be more concise, users can refer to docs for in depth explanations.
There was a problem hiding this comment.
Is this better?
:c:func:`PyErr_CheckSignals` has been made safe to call without holding the GIL.
It will acquire the GIL itself when it needs it.
|
I am unable to reproduce the failure of |
Compiled-code modules that implement time-consuming operations that don’t require manipulating Python objects, are supposed to call PyErr_CheckSignals frequently throughout each such operation, so that if the user interrupts the operation with control-C, it is cancelled promptly. In the normal case where no signals are pending, PyErr_CheckSignals is cheap; however, callers must hold the GIL, and compiled-code modules that implement time-consuming operations are also supposed to release the GIL during each such operation. The overhead of reclaiming the GIL in order to call PyErr_CheckSignals, and then releasing it again, sufficiently often for reasonable user responsiveness, can be substantial. If my understanding of the thread-state rules is correct, PyErr_CheckSignals only *needs* the GIL if it has work to do. *Checking* whether there is a pending signal, or a pending request to run the cycle collector, requires only a couple of atomic loads. Therefore: Reorganize the logic of PyErr_CheckSignals and its close relatives (_PyErr_CheckSignals and _PyErr_CheckSignalsTstate) so that all the “do we have anything to do” checks are done in a batch before anything that needs the GIL. If any of them are true, acquire the GIL, repeat the check (because another thread could have stolen the event while we were waiting for the GIL), and then actually do the work, enabling callers to *not* hold the GIL. (There are some fine details here that I’d really appreciate a second pair of eyes on — see the comments in the new functions _PyErr_CheckSignalsHoldingGIL and _PyErr_CheckSignalsNoGIL.)
The source tree contains dozens of loops of this form:
int res;
do {
Py_BEGIN_ALLOW_THREADS
res = some_system_call(arguments...);
Py_END_ALLOW_THREADS
} while (res < 0 && errno == EINTR && !PyErr_CheckSignals());
Now that it is possible to call PyErr_CheckSignals without holding the
GIL, the locking operations can be moved out of the loop:
Py_BEGIN_ALLOW_THREADS
do {
res = some_system_call(arguments...);
} while (res < 0 && errno == EINTR && !PyErr_CheckSignals());
Py_END_ALLOW_THREADS
This demonstrates the motivation for making it possible to call
PyErr_CheckSignals without holding the GIL. It shouldn’t make any
measurable difference performance-wise for _these_ loops, which almost
never actually cycle; but for loops that do cycle many times it’s very
much desirable to not take and release the GIL every time through.
In some cases I also moved uses of _Py_(BEGIN|END)_SUPPRESS_IPH, which
is often paired with Py_(BEGIN|END)_ALLOW_THREADS, to keep the pairing
intact. It was already considered safe to call PyErr_CheckSignals
from both inside and outside an IPH suppression region.
More could be done in this vein: I didn’t change any loops where the
inside of the loop was more complicated than a single system call,
_except_ that I did refactor py_getentropy and py_getrandom (in
bootstrap_hash.c) to make it possible to move the unlock and lock
outside the loop, demonstrating a more complicated case.
zackw
force-pushed
the
check-signals-without-gil
branch
from
40097b8 to
3e25a93
Compare
There was a problem hiding this comment.
I haven't fully decided how I feel about this yet. I agree with the motivation, but PyGILState_Ensure is evil. We might be able to sidestep most of the issues here, though (for one, signal handling isn't done in subinterpreters, so we don't have to worry about interpreter-guessing issues).
My main concern is that we're changing something that's in the stable ABI. That's generally a big no-no, because those are supposed to have a "frozen" interface. We might want this in a new API (e.g., something like PyErr_CheckSignalsFast).
| /* FIXME: Given that we already have 'tstate', is there a more efficient | ||
| way to do this? */ |
There was a problem hiding this comment.
Yes, you want PyThreadState_Swap(tstate).
| Determine whether there is actually any work needing to be done. | ||
| If so, acquire the GIL if necessary, and do that work. */ | ||
| static int | ||
| _PyErr_CheckSignalsNoGIL(PyThreadState *tstate, bool cycle_collect) |
There was a problem hiding this comment.
Bikeshedding: let's call this "NoTstate", because you'll still need a thread state on free-threaded builds.
| /* If this thread does not have a thread state at all, then it has | ||
| never been associated with the Python runtime, so it should not | ||
| attempt to handle signals or run the cycle collector. */ | ||
| if (!tstate) { |
There was a problem hiding this comment.
I'm not sure we should ignore this. This seems like blatant misuse--either return a failure or emit a fatal error.
|
TSan failure looks unrelated. I've restarted the job for you. |
Addresses #133465. See there, or the commit message for the first commit in this PR, for rationale.
There are two commits: the first actually implements the change, and the second demonstrates the motivation for it by pulling a lot of uses of
Py_(BEGIN|END)_ALLOW_THREADSwithin Python's stdlib out of loops.This has been tested (lightly - just the built in testsuite) both with and without
--disable-gil; however, I did not test--enable-optimizationsnor--enable-experimental-jit.📚 Documentation preview 📚: https://cpython-previews--133466.org.readthedocs.build/