Review comments from Sam

python · colesbury · Feb 20, 2024 · Jan 18, 2024 · Feb 9, 2024 · Feb 9, 2024
commit f78c505783ba5961734f104e3b95384e9cabb807
@@ -81,13 +81,8 @@ 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.
-     *
-     * It is by far the hottest field in this struct and should be placed at
-     * the beginning. */
+     * of truth when a thread is running. */
     uintptr_t interp_eval_breaker;
-    /* Avoid false sharing */
-    int64_t padding[7];
     int recursion_limit;
     struct _gil_runtime_state *gil;
     int own_gil;

diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
@@ -1768,6 +1768,7 @@ PyErr_CheckSignals(void)
        allows long running native code to clean cycles created using the C-API
        even if it doesn't run the evaluation loop */
     if (_PyThreadState_IsSignalled(tstate, _PY_GC_SCHEDULED_BIT)) {
+        _PyThreadState_Unsignal(tstate, _PY_GC_SCHEDULED_BIT);
         _Py_RunGC(tstate);
     }
 

@@ -114,7 +114,8 @@ update_thread_eval_breaker(PyInterpreterState *interp, PyThreadState *tstate)
 // 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) {
+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 func
8000
tion doesn't apply.
@@ -661,7 +662,7 @@ PyEval_RestoreThread(PyThreadState *tstate)
 */
 
 void
-_PyEval_SignalReceived()
+_PyEval_SignalReceived(void)
 {
     _PyThreadState_Signal(_PyRuntime.main_tstate, _PY_SIGNALS_PENDING_BIT);
 }
@@ -715,6 +716,7 @@ _pop_pending_call(struct _pending_calls *pending,
     }
 }
 
+#ifndef Py_GIL_DISABLED
 static void
 signal_active_thread(PyInterpreterState *interp, uintptr_t bit)
 {
@@ -732,6 +734,7 @@ signal_active_thread(PyInterpreterState *interp, uintptr_t bit)
     }
     MUTEX_UNLOCK(gil->mutex);
 }
+#endif
 
 /* This implementation is thread-safe.  It allows
    scheduling to be made from any thread, and even from an executing
@@ -756,7 +759,8 @@ _PyEval_AddPendingCall(PyInterpreterState *interp,
 
     if (main_only) {
         _PyThreadState_Signal(_PyRuntime.main_tstate, _PY_CALLS_TO_DO_BIT);
-    } else {
+    }
+    else {
 #ifdef Py_GIL_DISABLED
         _PyInterpreterState_SignalAll(interp, _PY_CALLS_TO_DO_BIT);
 #else
@@ -1014,7 +1018,6 @@ _PyEval_InitState(PyInterpreterState *interp)
 int
 _Py_HandlePending(PyThreadState *tstate)
 {
-    PyInterpreterState *interp = tstate->interp;
     uintptr_t breaker = _Py_atomic_load_uintptr_relaxed(&tstate->eval_breaker);
 
     /* Stop-the-world */
@@ -1042,6 +1045,7 @@ _Py_HandlePending(PyThreadState *tstate)
 
     /* GC scheduled to run */
     if ((breaker & _PY_GC_SCHEDULED_BIT) != 0) {
+        _PyThreadState_Unsignal(tstate, _PY_GC_SCHEDULED_BIT);
         _Py_RunGC(tstate);
     }
 

diff --git a/Python/gc.c b/Python/gc.c
@@ -1796,7 +1796,6 @@ _PyObject_GC_Link(PyObject *op)
 void
 _Py_RunGC(PyThreadState *tstate)
 {
-    _PyThreadState_Unsignal(tstate, _PY_GC_SCHEDULED_BIT);
     gc_collect_main(tstate, GENERATION_AUTO, _Py_GC_REASON_HEAP);
 }
 

diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c
@@ -1482,7 +1482,7 @@ PyObject_IS_GC(PyObject *obj)
 void
 _Py_ScheduleGC(PyThreadState *tstate)
 {
-    _PyInterpreterState_SignalAll(tstate->interp, _PY_GC_SCHEDULED_BIT);
+    _PyThreadState_Signal(tstate, _PY_GC_SCHEDULED_BIT);
 }
 
 void
@@ -1502,7 +1502,6 @@ _PyObject_GC_Link(PyObject *op)
 void
 _Py_RunGC(PyThreadState *tstate)
 {
-    _PyInterpreterState_UnsignalAll(tstate->interp, _PY_GC_SCHEDULED_BIT);
     gc_collect_main(tstate, 0, _Py_GC_REASON_HEAP);
 }
 

diff --git a/Python/instrumentation.c b/Python/instrumentation.c
@@ -898,7 +898,7 @@ static void
 set_version_raw(uintptr_t *breaker, uint32_t version)
 {
     uintptr_t old = _Py_atomic_load_uintptr(breaker);
-    intptr_t new;
+    uintptr_t new;
     do {
         new = (old & _PY_EVAL_EVENTS_MASK) | version;
     } while (!_Py_atomic_compare_exchange_uintptr(breaker, &old, new));
@@ -909,13 +909,14 @@ set_global_version(PyThreadState *tstate, uint32_t version)
 {
     assert((version & _PY_EVAL_EVENTS_MASK) == 0);
     PyInterpreterState *interp = tstate->interp;
-    set_version_raw(&tstate->interp->ceval.interp_eval_breaker, version);
+    set_version_raw(&interp->ceval.interp_eval_breaker, version);
 
 #ifdef Py_GIL_DISABLED
     // Set the version on all threads in free-threaded builds.
     _PyRuntimeState *runtime = &_PyRuntime;
     HEAD_LOCK(runtime);
-    for (; tstate; tstate = PyThreadState_Next(tstate)) {
+    for (tstate = interp->threads.head; tstate;
+         tstate = PyThreadState_Next(tstate)) {
         set_version_raw(&tstate->eval_breaker, version);
     };
     HEAD_UNLOCK(runtime);

@@ -1306,7 +1306,8 @@ init_threadstate(_PyThreadStateImpl *_tstate,
 
     assert(interp != NULL);
     tstate->interp = interp;
-    tstate->eval_breaker = interp->ceval.interp_eval_breaker;
+    tstate->eval_breaker =
+        _Py_atomic_load_uintptr_relaxed(&interp->ceval.interp_eval_breaker);
 
     // next/prev are set in add_threadstate().
     assert(tstate->next == NULL);
-Original file line number
+Diff line change
@@ Expand Up / @@ -1796,7 +1796,6 @@ _PyObject_GC_Link(PyObject *op) @@
     void
     _Py_RunGC(PyThreadState *tstate)
     {
-        _PyThreadState_Unsignal(tstate, _PY_GC_SCHEDULED_BIT);
         gc_collect_main(tstate, GENERATION_AUTO, _Py_GC_REASON_HEAP);
     }
@@ Expand Down @@