Skip to content

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

Merged
merged 13 commits into from
Feb 20, 2024

Conversation

swtaarrs
Copy link
Member

@swtaarrs swtaarrs commented Feb 9, 2024

This change adds an eval_breaker field to PyThreadState, renaming the existing PyInterpreterState.eval_breaker to instrumentation_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, since make_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 be eval_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 in instrumentation_version, and then:

  • Normal builds update the version in the current thread's eval_breaker. When a new thread takes the GIL, it copies the current version from instrumentation_version.
  • 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 mostly intended to maintain the status quo in normal builds for now.

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 atomic or/and, rather than a loop around an atomic compare/exchange like the old code.

  • Issue: Move the eval_breaker to PyThreadState #112175

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).
Copy link
Contributor

@colesbury colesbury left a 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)).

@swtaarrs
Copy link
Member Author

swtaarrs commented Feb 9, 2024

It doesn't matter much if we get de-scheduled -- other threads will see that a GC needs to be scheduled soon enough.

Thanks, that makes sense. I forgot that every subsequent allocation will keep checking the thresholds even once the GC is scheduled.

I don't think we need to copy the _PY_GC_SCHEDULED_BIT back to the interpreter eval breaker

If Mark agrees, then the only thing left in interp_eval_breaker will be the instrumentation version, and it might be worth renaming it and/or considering larger changes to how it works.

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)).

This line should do that already in all build types, although I realized that I should change it to read interp_eval_breaker atomically. init_threadstate() is only called by new_threadstate(), and it's called with the runtime lock held.

@swtaarrs
Copy link
Member Author

swtaarrs commented Feb 9, 2024

I think I've addressed all of Sam's comments except for removing the eval_breaker <-> interp_eval_breaker GC bit transfer for normal builds. I'll wait to hear from @markshannon before doing that.

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.

@markshannon
Copy link
Member

I'll wait to hear from @markshannon before doing that.

What do you want to know?

@markshannon
Copy link
Member

If Mark agrees, then the only thing left in interp_eval_breaker will be the instrumentation version, and it might be worth renaming it and/or considering larger changes to how it works.

Feel free to rename it. How did you want to change in how it works?

@swtaarrs
Copy link
Member Author

swtaarrs commented Feb 12, 2024

I'll wait to hear from @markshannon before doing that.

What do you want to know?

If you have thoughts on Sam's suggestions around _PY_GC_SCHEDULED_BIT in this comment. Basically: do we need to worry about a thread dropping the GIL after scheduling a GC (but before running the GC), or can we just assume that the next thread that takes the GIL will schedule a GC for itself soon anyway?

If Mark agrees, then the only thing left in interp_eval_breaker will be the instrumentation version, and it might be worth renaming it and/or considering larger changes to how it works.

Feel free to rename it. How did you want to change in how it works?

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.

@markshannon
Copy link
Member

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.

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.
So wouldn't _PY_GC_SCHEDULED_BIT always be zero when swapping threads as we would have just run the GC and nothing could have triggered another one?

@colesbury
Copy link
Contributor

We also can switch threads any time the GIL is released, such as by a PyEval_SaveThread() call before a blocking operation.

This means that the instrumentation version was the only thing left in
`interp_eval_breaker`, so I renamed it and performed some related
simplification.
Copy link
Contributor

@colesbury colesbury left a 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"
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@colesbury colesbury left a 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.

@swtaarrs
Copy link
Member Author

I resolved the merge conflicts - they were pretty straightforward. This should be good to go now (I replied inline about not making the uint32_t change in this PR).

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Brett!

@colesbury colesbury merged commit 0749244 into python:main Feb 20, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
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.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
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.
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants