gh-133465: Allow PyErr_CheckSignals to be called without holding the GIL. #133466
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.)
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| :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.) | ||
|
Comment on lines
+1
to
+7
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? There was a problem hiding this comment.
@StanFromIreland, I disagree. AFAIK you aren't a core dev or triager, I wish you wouldn't give other contributors questionable advice without a reference in such an authoritive-sounding way. There was a problem hiding this comment. I would update this and focus on how the new API differs from the old one: IIUC, "can be called without the GIL [or whatever PC phrasing] and doesn't acquire it unless a signal's handler needs to be called." |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1759,12 +1759,22 @@ _PySignal_Fini(void) | |||||||
| Py_CLEAR(state->ignore_handler); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Subroutine of _PyErr_CheckSignalsNoGIL. Does all the work that | ||||||||
| needs the GIL. When called, 'tstate' must be the thread state for | ||||||||
| the current thread, and the current thread must hold the GIL. */ | ||||||||
| static int | ||||||||
| _PyErr_CheckSignalsHoldingGIL(PyThreadState *tstate, bool cycle_collect) | ||||||||
| { | ||||||||
|
There was a problem hiding this comment. Possibly start with |
||||||||
| #if defined(Py_REMOTE_DEBUG) && defined(Py_SUPPORTS_REMOTE_DEBUG) | ||||||||
| _PyRunRemoteDebugger(tstate); | ||||||||
| #endif | ||||||||
|
|
||||||||
| /* It is necessary to repeat all of the checks of global flags | ||||||||
| that were done in _PyErr_CheckSignalsNoGIL. At the time of | ||||||||
|
There was a problem hiding this comment. That function does not exist -- not sure what you meant. |
||||||||
| those checks, we might not have held the GIL; in between those | ||||||||
| checks and when we acquired the GIL, some other thread may have | ||||||||
| processed the events that were flagged. Since we now hold the | ||||||||
|
Comment on lines
+1776
to
+1777
There was a problem hiding this comment. No other thread could have processed the events (only the main thread of the main interpreter can) but new events might have been added.
Suggested change
|
||||||||
| GIL, a check now will be valid until we release it again. */ | ||||||||
|
|
||||||||
| /* Opportunistically check if the GC is scheduled to run and run it | ||||||||
| if we have a request. This is done here because native code needs | ||||||||
|
|
@@ -1777,24 +1787,8 @@ PyErr_CheckSignals(void) | |||||||
| _Py_RunGC(tstate); | ||||||||
| } | ||||||||
|
|
||||||||
| if (!_Py_ThreadCanHandleSignals(tstate->interp) | ||||||||
| || !_Py_atomic_load_int(&is_tripped)) { | ||||||||
| return 0; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -1877,15 +1871,66 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate) | |||||||
| return 0; | ||||||||
| } | ||||||||
|
|
||||||||
| /* Subroutine of the PyErr_CheckSignals family: | ||||||||
| 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. There was a problem hiding this comment. It's a little weird to name a function (Also, given that these are static functions, possibly they should be named more like There was a problem hiding this comment. Sure, whatever. Just don't call it "GIL" :) |
||||||||
| { | ||||||||
| /* 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. There was a problem hiding this comment. All callers of this function get the tstate argument from There was a problem hiding this comment.
There was a problem hiding this comment. Looks all right to me in the new version of the PR -- the new function might be called under those circumstances, and no tstate means that it's definitely not the main thread of the main interpreter, so ignoring the call feels like the right thing to do. Right? |
||||||||
| return 0; | ||||||||
| } | ||||||||
|
|
||||||||
| _Py_CHECK_EMSCRIPTEN_SIGNALS(); | ||||||||
|
|
||||||||
| /* Don't acquire the GIL if we don't have anything to do. | ||||||||
| VERIFYME: I *think* every piece of this expression is safe to | ||||||||
| execute without holding the GIL and is already sufficiently | ||||||||
| atomic. */ | ||||||||
|
Comment on lines
+1928
to
+1930
There was a problem hiding this comment. I double checked and it looks safe to me, so you can drop this comment. |
||||||||
| if ((!_Py_ThreadCanHandleSignals(tstate->interp) | ||||||||
|
There was a problem hiding this comment. This is missing a check for the remote debugger interface. Otherwise this will not run the remote debugger code when users call this function without the GIL There was a problem hiding this comment. The checks are the same ones that |
||||||||
| || !_Py_atomic_load_int(&is_tripped)) | ||||||||
| && (!cycle_collect | ||||||||
| || !_Py_eval_breaker_bit_is_set(tstate, _PY_GC_SCHEDULED_BIT))) { | ||||||||
| return 0; | ||||||||
| } | ||||||||
|
|
||||||||
| /* FIXME: Given that we already have 'tstate', is there a more efficient | ||||||||
| way to do this? */ | ||||||||
|
Comment on lines
+1938
to
+1939
There was a problem hiding this comment. Yes, you want There was a problem hiding this comment. The documentation for that function says "Swap the current thread state with the thread state given by the argument tstate, which may be NULL. The global interpreter lock must be held and is not released." So that really doesn't sound like what I should be using here. What I think I need is more like There was a problem hiding this comment. The 3.13 docs for Ignore the phrase "holds the GIL". Read it as "hold an attached thread state". You always need a thread state to call the C API, in both FT and GIL-icious builds. |
||||||||
| PyGILState_STATE st = PyGILState_Ensure(); | ||||||||
| int err = _PyErr_CheckSignalsHoldingGIL(tstate, cycle_collect); | ||||||||
| PyGILState_Release(st); | ||||||||
|
|
||||||||
| return err; | ||||||||
| } | ||||||||
|
|
||||||||
| /* Declared in pycore_pyerrors.h. | ||||||||
| This function may be called without the GIL. */ | ||||||||
| int | ||||||||
| _PyErr_CheckSignalsTstate(PyThreadState *tstate) | ||||||||
| { | ||||||||
| return _PyErr_CheckSignalsNoGIL(tstate, true); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Declared in pycore_pyerrors.h. | ||||||||
| This function may be called without the GIL. */ | ||||||||
| int | ||||||||
| _PyErr_CheckSignals(void) | ||||||||
| { | ||||||||
| PyThreadState *tstate = PyGILState_GetThisThreadState(); | ||||||||
| return _PyErr_CheckSignalsNoGIL(tstate, false); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Declared in pyerrors.h. | ||||||||
| This function may be called without the GIL. */ | ||||||||
| int | ||||||||
| PyErr_CheckSignals(void) | ||||||||
| { | ||||||||
| PyThreadState *tstate = PyGILState_GetThisThreadState(); | ||||||||
| return _PyErr_CheckSignalsNoGIL(tstate, true); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Simulate the effect of a signal arriving. The next time PyErr_CheckSignals | ||||||||
| is called, the corresponding Python signal handler will be raised. | ||||||||
|
|
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.
That is too frequent for my taste. Use interactions feel "delay-less" under 10 msec, and for ^C 100 even msec feels very quick, and 1sec would still be acceptable. After a few seconds I would hit ^C again.
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.
This is briefly discussed in the talk. If you add enough
PyErr_CheckSignalscalls to your extension module that every long-running loop can be interrupted, in practice this makes you do the check way too often, once every few tens of micro seconds. This is fine with the hypothetical newPyErr_CheckSignals_Detached, but with the old version, where you have to reclaim the GIL, it's way too costly. So in the talk I recommended looking at the actual system clock (withclock_gettime) and only callingPyErr_CheckSignalsif a millisecond or more had gone by. That's faster than required for human responsiveness, yes, but the remaining overhead is the overhead of looking at the clock and you can't reduce that by doing the actual calls even less often. So that's where "at least once a millisecond" came from.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.
Maybe explain that better in the docs then.