Don't copy _PY_GC_SCHEDULED_BIT to/from interp_eval_breaker

This means that the instrumentation version was the only thing left in `interp_eval_breaker`, so I renamed it and performed some related simplification.
python · colesbury · Feb 20, 2024 · Jan 18, 2024 · Feb 9, 2024 · Feb 9, 2024
commit 426041007c447f7b4141a9ecb794a022d32d5e58
@@ -78,11 +78,10 @@ struct _ceval_runtime_state {
 
 
 struct _ceval_state {
-    /* This single variable holds the global instrumentation version and some
-     * interpreter-global requests to break out of the fast path in the eval
-     * loop. PyThreadState also contains an eval_breaker, which is the source
-     * of truth when a thread is running. */
-    uintptr_t interp_eval_breaker;
+    /* This variable holds the global instrumentation version. When a thread is
+       running, this value is overlaid onto PyThreadState.eval_breaker so that
+       changes in the instrumentation version will trigger the eval breaker. */
+    uintptr_t instrumentation_version;
     int recursion_limit;
     struct _gil_runtime_state *gil;
     int own_gil;

@@ -56,7 +56,7 @@
 #define _Py_atomic_load_relaxed_int32(ATOMIC_VAL) _Py_atomic_load_relaxed(ATOMIC_VAL)
 #endif
 
-// Atomically copy the bits indicated by mask between two eval breakers.
+// Atomically copy the bits indicated by mask between two values.
 static inline void
 copy_eval_breaker_bits(uintptr_t *from, uintptr_t *to, uintptr_t mask)
 {
@@ -73,11 +73,10 @@ copy_eval_breaker_bits(uintptr_t *from, uintptr_t *to, uintptr_t mask)
     } while (!_Py_atomic_compare_exchange_uintptr(to, &old_value, new_value));
 }
 
-// When attaching a thread, set the global instrumentation version,
-// _PY_CALLS_TO_DO_BIT, and _PY_GC_SCHEDULED_BIT to match the current state of
-// the interpreter.
+// When attaching a thread, set the global instrumentation version and
+// _PY_CALLS_TO_DO_BIT from the current state of the interpreter.
 static inline void
-update_thread_eval_breaker(PyInterpreterState *interp, PyThreadState *tstate)
+update_eval_breaker_for_thread(PyInterpreterState *interp, PyThreadState *tstate)
 {
 #ifdef Py_GIL_DISABLED
     // Free-threaded builds eagerly update the eval_breaker on *all* threads as
@@ -103,32 +102,10 @@ update_thread_eval_breaker(PyInterpreterState *interp, PyThreadState *tstate)
     }
 
     // _PY_CALLS_TO_DO_BIT was derived from other state above, so the only bits
-    // we copy from our interpreter's eval_breaker are the instrumentation
-    // version number and GC bit.
-    const uintptr_t mask = ~_PY_EVAL_EVENTS_MASK | _PY_GC_SCHEDULED_BIT;
-    copy_eval_breaker_bits(&interp->ceval.interp_eval_breaker,
+    // we copy from our interpreter's state are the instrumentation version.
+    copy_eval_breaker_bits(&interp->ceval.instrumentation_version,
                            &tstate->eval_breaker,
-                           mask);
-}
-
-// When detaching a thread, transfer _PY_GC_SCHEDULED_BIT to its interpreter,
-// in case a GC was scheduled but not processed yet.
-static inline void
-update_interp_eval_breaker(PyThreadState *tstate, PyInterpreterState *interp)
-{
-#ifdef Py_GIL_DISABLED
-    // Free-threaded builds eagerly update the eval_breaker on *all* threads as
-    // needed, so this function doesn't apply.
-    return;
-#endif
-
-    if (tstate == NULL) {
-        return;
-    }
-
-    copy_eval_breaker_bits(&tstate->eval_breaker,
-                           &interp->ceval.interp_eval_breaker,
-                           _PY_GC_SCHEDULED_BIT);
+                           ~_PY_EVAL_EVENTS_MASK);
 }
 
 /*
@@ -259,7 +236,6 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate)
     }
 
     MUTEX_LOCK(gil->mutex);
-    update_interp_eval_breaker(tstate, interp);
     _Py_ANNOTATE_RWLOCK_RELEASED(&gil->locked, /*is_write=*/1);
     _Py_atomic_store_int_relaxed(&gil->locked, 0);
     COND_SIGNAL(gil->cond);
@@ -400,7 +376,7 @@ take_gil(PyThreadState *tstate)
     assert(_PyThreadState_CheckConsistency(tstate));
 
     _Py_unset_eval_breaker_bit(tstate, _PY_GIL_DROP_REQUEST_BIT);
-    update_thread_eval_breaker(interp, tstate);
+    update_eval_breaker_for_thread(interp, tstate);
 
     MUTEX_UNLOCK(gil->mutex);
 

diff --git a/Python/instrumentation.c b/Python/instrumentation.c
@@ -891,25 +891,27 @@ static inline int most_significant_bit(uint8_t bits) {
 static uint32_t
 global_version(PyInterpreterState *interp)
 {
-    return interp->ceval.interp_eval_breaker & ~_PY_EVAL_EVENTS_MASK;
+    return _Py_atomic_load_uintptr_relaxed(&interp->ceval.instrumentation_version);
 }
 
+/* Atomically set the given version in the given location, without touching
+   anything in _PY_EVAL_EVENTS_MASK. */
 static void
-set_version_raw(uintptr_t *breaker, uint32_t version)
+set_version_raw(uintptr_t *ptr, uint32_t version)
 {
-    uintptr_t old = _Py_atomic_load_uintptr(breaker);
+    uintptr_t old = _Py_atomic_load_uintptr_relaxed(ptr);
     uintptr_t new;
     do {
         new = (old & _PY_EVAL_EVENTS_MASK) | version;
-    } while (!_Py_atomic_compare_exchange_uintptr(breaker, &old, new));
+    } while (!_Py_atomic_compare_exchange_uintptr(ptr, &old, new));
 }
 
 static void
 set_global_version(PyThreadState *tstate, uint32_t version)
 {
     assert((version & _PY_EVAL_EVENTS_MASK) == 0);
     PyInterpreterState *interp = tstate->interp;
-    set_version_raw(&interp->ceval.interp_eval_breaker, version);
+    set_version_raw(&interp->ceval.instrumentation_version, version);
 
 #ifdef Py_GIL_DISABLED
     // Set the version on all threads in free-threaded builds.
@@ -921,7 +923,7 @@ set_global_version(PyThreadState *tstate, uint32_t version)
     };
     HEAD_UNLOCK(runtime);
 #else
-    // Normal builds take the current version from interp_eval_breaker when
+    // Normal builds take the current version from instrumentation_version when
     // attaching a thread, so we only have to set the current thread's version.
     set_version_raw(&tstate->eval_breaker, version);
 #endif
@@ -1588,7 +1590,7 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
 {
     if (is_version_up_to_date(code, interp)) {
         assert(
-            (interp->ceval.interp_eval_breaker & ~_PY_EVAL_EVENTS_MASK) == 0 ||
+            interp->ceval.instrumentation_version == 0 ||
             instrumentation_cross_checks(interp, code)
         );
         return 0;

@@ -794,9 +794,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
 
     Py_CLEAR(interp->audit_hooks);
 
-    // At this time, all the threads should be cleared so we don't need
-    // atomic operations for eval_breaker
-    interp->ceval.interp_eval_breaker = 0;
+    interp->ceval.instrumentation_version = 0;
 
     for (int i = 0; i < _PY_MONITORING_UNGROUPED_EVENTS; i++) {
         interp->monitors.tools[i] = 0;
@@ -1310,7 +1308,7 @@ init_threadstate(_PyThreadStateImpl *_tstate,
     assert(interp != NULL);
     tstate->interp = interp;
     tstate->eval_breaker =
-        _Py_atomic_load_uintptr_relaxed(&interp->ceval.interp_eval_breaker);
+        _Py_atomic_load_uintptr_relaxed(&interp->ceval.instrumentation_version);
 
     // next/prev are set in add_threadstate().
     assert(tstate->next == NULL);