-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-112175: Add eval_breaker
to PyThreadState
#115194
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
Conversation
This change adds an `eval_breaker` field to `PyThreadState`, renaming the existing `eval_breaker` to `interp_eval_breaker` (its uses are explained further down). The primary motivation is for performance in free-threaded builds: with thread-local eval breakers, we can stop a specific thread (e.g., for an async exception) without interrupting other threads. There are still two situations where we want the first available thread to handle a request: - Running a garbage collection: In normal builds, we set `_PY_GC_SCHEDULED_BIT` on the current thread. In case a thread suspends before handling the collection, the bit is copied to and from `interp_eval_breaker` on thread suspend and resume, respectively. In a free-threaded build, we simply iterate over all threads and set the bit. The first thread to check its eval breaker runs the collection, unsetting the bit on all threads. - Free-threaded builds could have multiple threads attempt a GC from one trigger if we get very unlucky with thread scheduling. I didn't put any protections against this in place because a) the consequences of it happening are just that one or more threads will check the GC thresholds right after a collection finishes, which won't affect correctness and b) it's incredibly, vanishingly unlikely. - Pending calls not limited to the main thread (possible since python/cpython@757b402ea1c2). This is a little tricker, since the callback can be added from any thread, with or without the GIL held. If the targeted interpreter's GIL is locked, we signal the holding thread. When a thread is resumed, its `_PY_CALLS_TO_DO` bit is derived from the source of truth for pending calls (one of two `_pending_calls` structs). This handles situations where no thread held the GIL when the call was first added, or if the active thread did not handle the call before releasing the GIL. In a free-threaded build, all threads all signaled, similar to scheduling a GC. The source of truth for the global instrumentation version is still in `interp_eval_breaker`, in both normal and free-threaded builds. Threads usually read the version from their local `eval_breaker`, where it continues to be colocated with the eval breaker bits, and the method for keeping it up to date depends on build type. All builds first update the version in `interp_eval_breaker`, and then: - Normal builds update the version in the current thread's `eval_breaker`. When a thread takes the GIL, it copies the current version from `interp_eval_breaker` as part of the same operation that copies `_PY_GC_SCHEDULED_BIT`. - Free-threaded builds again iterate over all threads in the current interpreter, updating the version on each one. Instrumentation (and the specializing interpreter more generally) will need more work to be compatible with free-threaded builds, so these changes are just intended to maintain the status quo in normal builds for now. Other notable changes are: - The `_PY_*_BIT` macros now expand to the actual bit being set, rather than the bit's index. I think this is simpler overall. I also moved their definitions from `pycore_ceval.h` to `pycore_pystate.h`, since their main usage is on `PyThreadState`s now. - Most manipulations of `eval_breaker` are done with a new pair of functions: `_PyThreadState_Signal()` and `_PyThreadState_Unsignal()`. Having two separate functions to set/unset a bit, rather than one function that takes the bit value to use, lets us use a single atomic `or`/`and`, rather than a loop around an atomic compare/exchange like the old `_Py_set_eval_breaker_bit` function. Existing tests provide pretty good coverage for most of this functionality. The one new test I added is to make sure a GC still happens if a thread schedules it then drops the GIL before the GC runs. I don't love how complicated this test ended up so I'm open to other ideas for how to test this (or other things to test in general).
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.
Thanks Brett! This looks good overall, but the free-threaded GitHub CI is failing and there are a few warnings that need to be addressed. I've left comments inline as well.
_PY_GC_SCHEDULED_BIT
For the free-threaded build, I think we only need to set the GC scheduled bit on the current thread. It doesn't matter much if we get de-scheduled -- other threads will see that a GC needs to be scheduled soon enough. Regarding "multiple threads attempt a GC": that can be a performance issue once the GIL is disabled, but will be addressed in gh-114880.
I think the same logic applies to the default build, but I'd like Mark's opinion on it. In other words, I don't think we need to copy the _PY_GC_SCHEDULED_BIT
back to the interpreter eval breaker. In the rare case where we get swapped out between setting _PY_GC_SCHEDULED_BIT
and running the GC, the newly scheduled thread will figure out that it needs to run the GC after the next GC allocation.
Instrumentation
In the free-threaded build, I think we'll also need to copy the instrumentation bits when we create a new thread (while holding HEAD_LOCK(runtime)
).
Thanks, that makes sense. I forgot that every subsequent allocation will keep checking the thresholds even once the GC is scheduled.
If Mark agrees, then the only thing left in
This line should do that already in all build types, although I realized that I should change it to read |
Minor conflicts from the new _PY_EVAL_EXPLICIT_MERGE_BIT flag.
I think I've addressed all of Sam's comments except for removing the One of the free-threading test failures was the new gc scheduling test, which I deleted in anticipation of removing that functionality. I couldn't repro any of the other failures on either my Linux or MacOS development machines; if the same tests fail again in CI I'll dig further. |
What do you want to know? |
Feel free to rename it. How did you want to change in how it works? |
If you have thoughts on Sam's suggestions around
I didn't have any specific changes in mind, other than making sure we take any opportunities to simplify things once it contains only the version number. We should still keep the version number left-shifted out of the way of the eval breaker bits, since the version is combined with the bits in thread-local eval breakers. Other than that I can't think of anything. |
Correct me if I'm wrong, but we copy the bits from thread state to interpreter state when we are switching threads, and that only happens in the eval breaker handler, which also runs the GC. |
We also can switch threads any time the GIL is released, such as by a |
This means that the instrumentation version was the only thing left in `interp_eval_breaker`, so I renamed it and performed some related simplification.
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.
Looks pretty good, a few comments below
@@ -5,7 +5,7 @@ | |||
#include "Python.h" | |||
#include "pycore_abstract.h" // _PyIndex_Check() | |||
#include "pycore_call.h" // _PyObject_CallNoArgs() | |||
#include "pycore_ceval.h" // _PyEval_SignalAsyncExc() | |||
#include "pycore_ceval.h" |
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 this change intentional? Otherwise, should probably revert it.
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.
It was intentional: I removed the _PyEval_SignalAsyncExc()
function and replaced the one call to it (which wasn't even in this file) with _Py_set_eval_breaker_bit(...)
. I tried removing the #include
and saw that a number of other functions from pycore_ceval.h
are still used.
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 looks good to me other than the merge conflicts. Those were probably from conflicts with my PRs -- sorry about that.
Did you want to make instrumentation_version
a uint32_t
now that it doesn't include the eval breaker? Either way seems fine to me.
I resolved the merge conflicts - they were pretty straightforward. This should be good to go now (I replied inline about not making the |
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.
Thanks Brett!
This change adds an `eval_breaker` field to `PyThreadState`. The primary motivation is for performance in free-threaded builds: with thread-local eval breakers, we can stop a specific thread (e.g., for an async exception) without interrupting other threads. The source of truth for the global instrumentation version is stored in the `instrumentation_version` field in PyInterpreterState. Threads usually read the version from their local `eval_breaker`, where it continues to be colocated with the eval breaker bits.
This change adds an `eval_breaker` field to `PyThreadState`. The primary motivation is for performance in free-threaded builds: with thread-local eval breakers, we can stop a specific thread (e.g., for an async exception) without interrupting other threads. The source of truth for the global instrumentation version is stored in the `instrumentation_version` field in PyInterpreterState. Threads usually read the version from their local `eval_breaker`, where it continues to be colocated with the eval breaker bits.
This change adds an `eval_breaker` field to `PyThreadState`. The primary motivation is for performance in free-threaded builds: with thread-local eval breakers, we can stop a specific thread (e.g., for an async exception) without interrupting other threads. The source of truth for the global instrumentation version is stored in the `instrumentation_version` field in PyInterpreterState. Threads usually read the version from their local `eval_breaker`, where it continues to be colocated with the eval breaker bits.
This change adds an
eval_breaker
field toPyThreadState
, renaming the existingPyInterpreterState.eval_breaker
toinstrumentation_version
(explained further down). The primary motivation is for performance in free-threaded builds: with thread-local eval breakers, we can stop a specific thread (e.g., for an async exception) without interrupting other threads.Pending Calls
We still want to signal all threads in an interpreter for pending calls that aren't limited to the main thread (possible since 757b402ea1c2). These callbacks can be added by any thread, with or without the GIL held. If the targeted interpreter's GIL is locked, we signal the holding thread. When any thread is resumed, its
_PY_CALLS_TO_DO
bit is derived from the source of truth for pending calls (one of two_pending_calls
structs). This handles situations where no thread held the GIL when the call was first added, or if the active thread did not handle the call before releasing the GIL.In a free-threaded build, all threads in the targeted interpreter are signaled. This is slightly less efficient, both because we have to iterate over all threads to set the bit, and because it's more likely that multiple threads could call into
make_pending_calls()
before the bit is cleared. This shouldn't affect correctness, though, sincemake_pending_calls()
and the_pending_calls
structs have appropriate synchronization in place to ensure each callback happens once.Instrumentation Version
The source of truth for the global instrumentation version is still in
PyInterpreterState.instrumentation_version
(which used to beeval_breaker
), in both normal and free-threaded builds. Now that this variable contains only the instrumentation version (and no more eval breaker bits), some of the code to read and write it is simplified.Threads usually read the version from their local
eval_breaker
, where it continues to be colocated with the eval breaker bits, and the method for keeping it up to date depends on build type. Both builds first update the version ininstrumentation_version
, and then:eval_breaker
. When a new thread takes the GIL, it copies the current version frominstrumentation_version
.Other notable changes
The
_PY_*_BIT
macros now expand to the actual bit being set, rather than the bit's index. I think this is simpler overall._Py_set_eval_breaker_bit(interp, bit, set)
has been split into_Py_set_eval_breaker_bit(interp, bit)
and_Py_unset_eval_breaker_bit(interp, bit)
. This lets us use a single atomicor
/and
, rather than a loop around an atomic compare/exchange like the old code.Issue: Move the
eval_breaker
toPyThreadState
#112175