Address review

Author: Fidget-Spinner

Objects/codeobject.c

@@ -2434,9 +2434,7 @@ code_dealloc(PyObject *self)
 #ifdef _Py_TIER2
     _Py_Executors_InvalidateDependency(tstate->interp, self, 1);
     if (co->co_executors != NULL) {
-        Py_BEGIN_CRITICAL_SECTION(co);
         clear_executors(co);
-        Py_END_CRITICAL_SECTION();
     }
 #endif
 

Objects/funcobject.c

@@ -411,8 +411,9 @@ uint32_t
 _PyFunction_GetVersionForCurrentState(PyFunctionObject *func)
 {
     // This function does not need locking/atomics as it can only be
-    // called from the optimizer, which is currently disabled
-    // when there are multiple threads.
+    // called from the specializing interpreter or optimizer.
+    // The specializing interpreter holds a strong reference to the function.
+    // The optimizer is currently disabled when there are multiple threads.
     return func->func_version;
 }
 

Python/optimizer.c

@@ -241,8 +241,9 @@ get_oparg(PyObject *self, PyObject *Py_UNUSED(ignored))
 
 ///////////////////// Experimental UOp Optimizer /////////////////////
 
-static int executor_clear(PyObject *executor);
-static void unlink_executor(_PyExecutorObject *executor);
+static int executor_clear(PyInterpreterState *interp, PyObject *executor);
+static int executor_clear_implicit_interp(PyObject *executor);
+static void unlink_executor(PyInterpreterState *interp, _PyExecutorObject *executor);
 
 
 void
@@ -308,7 +309,7 @@ uop_dealloc(PyObject *op) {
     _PyExecutorObject *self = _PyExecutorObject_CAST(op);
     _PyObject_GC_UNTRACK(self);
     assert(self->vm_data.code == NULL);
-    unlink_executor(self);
+    unlink_executor(_PyInterpreterState_GET(), self);
     // Once unlinked it becomes impossible to invalidate an executor, so do it here.
     self->vm_data.valid = 0;
     add_to_pending_deletion_list(self);
@@ -464,7 +465,7 @@ PyTypeObject _PyUOpExecutor_Type = {
     .tp_as_sequence = &uop_as_sequence,
     .tp_methods = uop_executor_methods,
     .tp_traverse = executor_traverse,
-    .tp_clear = executor_clear,
+    .tp_clear = executor_clear_implicit_interp,
     .tp_is_gc = executor_is_gc,
 };
 
@@ -1525,7 +1526,7 @@ link_executor(_PyExecutorObject *executor)
 }
 
 static void
-unlink_executor(_PyExecutorObject *executor)
+unlink_executor(PyInterpreterState *interp, _PyExecutorObject *executor)
 {
     if (!executor->vm_data.linked) {
         return;
@@ -1542,7 +1543,6 @@ unlink_executor(_PyExecutorObject *executor)
     }
     else {
         // prev == NULL implies that executor is the list head
-        PyInterpreterState *interp = PyInterpreterState_Get();
         assert(interp->executor_list_head == executor);
         interp->executor_list_head = next;
     }
@@ -1653,15 +1653,19 @@ _Py_ExecutorDetach(_PyExecutorObject *executor)
     Py_DECREF(executor);
 }
 
+// Note: we must use the interp state supplied and pass it to
+// unlink_executor. This function might be called when a new
+// interpreter is being created/removed. Thus the interpreter
+// state is inconsistent with where the executor actually belongs.
 static int
-executor_clear(PyObject *op)
+executor_clear(PyInterpreterState *interp, PyObject *op)
 {
     _PyExecutorObject *executor = _PyExecutorObject_CAST(op);
     if (!executor->vm_data.valid) {
         return 0;
     }
     assert(executor->vm_data.valid == 1);
-    unlink_executor(executor);
+    unlink_executor(interp, executor);
     executor->vm_data.valid = 0;
 
     /* It is possible for an executor to form a reference
@@ -1675,15 +1679,21 @@ executor_clear(PyObject *op)
         executor->exits[i].temperature = initial_unreachable_backoff_counter();
         _PyExecutorObject *e = executor->exits[i].executor;
         executor->exits[i].executor = NULL;
-        if (e != cold && e != cold_dynamic) {
-            executor_clear((PyObject *)e);
+        if (e != cold && e != cold_dynamic && e->vm_data.code == NULL) {
+            executor_clear(interp, (PyObject *)e);
         }
     }
     _Py_ExecutorDetach(executor);
     Py_DECREF(executor);
     return 0;
 }
 
+static int
+executor_clear_implicit_interp(PyObject *op)
+{
+    return executor_clear(_PyInterpreterState_GET(), op);
+}
+
 void
 _Py_Executor_DependsOn(_PyExecutorObject *executor, void *obj)
 {
@@ -1728,7 +1738,7 @@ _Py_Executors_InvalidateDependency(PyInterpreterState *interp, void *obj, int is
     }
     for (Py_ssize_t i = 0; i < PyList_GET_SIZE(invalidate); i++) {
         PyObject *exec = PyList_GET_ITEM(invalidate, i);
-        executor_clear(exec);
+        executor_clear(interp, exec);
         if (is_invalidation) {
             OPT_STAT_INC(executors_invalidated);
         }
@@ -1766,7 +1776,7 @@ _Py_Executors_InvalidateAll(PyInterpreterState *interp, int is_invalidation)
             _PyCode_Clear_Executors(executor->vm_data.code);
         }
         else {
-            executor_clear((PyObject *)executor);
+            executor_clear(interp, (PyObject *)executor);
         }
         if (is_invalidation) {
             OPT_STAT_INC(executors_invalidated);
@@ -1801,7 +1811,7 @@ _Py_Executors_InvalidateCold(PyInterpreterState *interp)
     }
     for (Py_ssize_t i = 0; i < PyList_GET_SIZE(invalidate); i++) {
         PyObject *exec = PyList_GET_ITEM(invalidate, i);
-        executor_clear(exec);
+        executor_clear(interp, exec);
     }
     Py_DECREF(invalidate);
     return;

Python/optimizer_analysis.c

@@ -226,14 +226,14 @@ eliminate_pop_guard(_PyUOpInstruction *this_instr, bool exit)
 
 static JitOptRef
 lookup_attr(JitOptContext *ctx, _PyUOpInstruction *this_instr,
-            PyTypeObject *type, PyObject *name, uint16_t immortal,
+            PyTypeObject *type, PyObject *name, uint16_t deferred_refcount,
             uint16_t mortal)
 {
     // The cached value may be dead, so we need to do the lookup again... :(
     if (type && PyType_Check(type)) {
         PyObject *lookup = _PyType_Lookup(type, name);
         if (lookup) {
-            int opcode = _Py_IsImmortal(lookup) || _PyObject_HasDeferredRefcount(lookup) ? immortal : mortal;
+            int opcode = _Py_IsImmortal(lookup) || _PyObject_HasDeferredRefcount(lookup) ? deferred_refcount : mortal;
             REPLACE_OP(this_instr, opcode, 0, (uintptr_t)lookup);
             return sym_new_const(ctx, lookup);
         }

Python/pystate.c

@@ -1522,20 +1522,18 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate,
 {
     assert(interp->threads.head != tstate);
     if (next != NULL) {
+#if defined(_Py_TIER2) && defined(Py_GIL_DISABLED)
+        FT_ATOMIC_STORE_UINT8(interp->jit, 0);
+        // There's more than one thread. In FT mode,
+        // disable the JIT completely for now.
+        _Py_Executors_InvalidateAll(interp, 1);
+#endif
         assert(next->prev == NULL || next->prev == tstate);
         next->prev = tstate;
     }
     tstate->next = next;
     assert(tstate->prev == NULL);
     interp->threads.head = tstate;
-#if defined(_Py_TIER2) && defined(Py_GIL_DISABLED)
-    // There's more than one thread. In FT mode,
-    // disable the JIT completely for now.
-    if (next != NULL) {
-        _Py_Executors_InvalidateAll(interp, 1);
-        FT_ATOMIC_STORE_UINT8(interp->jit, 0);
-    }
-#endif
 }
 
 static PyThreadState *