gh-153946: Refine automatic GC deferral

Author: pablogsal

Include/internal/pycore_gc.h

@@ -331,11 +331,9 @@ extern PyObject *_PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs);
 // Functions to clear types free lists
 extern void _PyGC_ClearAllFreeLists(PyInterpreterState *interp);
 
-// These calls nest across all threads in an interpreter. Allocation counters
-// continue advancing, explicit collections remain enabled, and a collection
-// that becomes due remains eligible at the next normal scheduling opportunity.
-// An automatic collection already starting concurrently may emit callbacks,
-// but the free-threaded collector will not traverse the protected object graph.
+// Nesting is interpreter-wide. Explicit collections and allocation counting
+// continue, and resuming does not schedule a collection immediately.
+// An in-flight free-threaded collection may still emit start and stop callbacks.
 PyAPI_FUNC(void) _PyGC_DeferAutomaticCollection(PyThreadState *tstate);
 PyAPI_FUNC(void) _PyGC_ResumeAutomaticCollection(PyThreadState *tstate);
 extern void _Py_RunGC(PyThreadState *tstate);

Lib/test/test_gc.py

@@ -144,6 +144,7 @@ def test_defer_automatic_collection_nested(self):
                 gc.disable()
 
     @unittest.skipIf(_testinternalcapi is None, "requires _testinternalcapi")
+    @threading_helper.requires_working_threading()
     def test_defer_automatic_collection_across_threads(self):
         was_enabled = gc.isenabled()
         gc.enable()
@@ -158,6 +159,7 @@ def test_defer_automatic_collection_across_threads(self):
                             [] for _ in range(10_000)))
                     thread.start()
                     thread.join()
+                    self.assertEqual(len(objects), 10_000)
                     self.assertEqual(self.total_collections(), before)
                 finally:
                     _testinternalcapi.resume_automatic_gc()

Parser/asdl_c.py

@@ -2118,7 +2118,6 @@ class PartingShots(StaticVisitor):
         return NULL;
     }
     PyThreadState *tstate = _PyThreadState_GET();
-    // The new objects cannot be cyclic until the completed tree is returned.
     _PyGC_DeferAutomaticCollection(tstate);
     PyObject *result = ast2obj_mod(state, t);
     _PyGC_ResumeAutomaticCollection(tstate);

Python/Python-ast.c

@@ -1787,20 +1787,16 @@ void
 _PyGC_DeferAutomaticCollection(PyThreadState *tstate)
 {
     GCState *gcstate = &tstate->interp->gc;
-    int previous = _Py_atomic_add_int(
-        &gcstate->automatic_collection_pause_count, 1);
-    (void)previous;
-    assert(previous >= 0);
+    assert(gcstate->automatic_collection_pause_count >= 0);
+    gcstate->automatic_collection_pause_count++;
 }
 
 void
 _PyGC_ResumeAutomaticCollection(PyThreadState *tstate)
 {
     GCState *gcstate = &tstate->interp->gc;
-    int previous = _Py_atomic_add_int(
-        &gcstate->automatic_collection_pause_count, -1);
-    (void)previous;
-    assert(previous > 0);
+    assert(gcstate->automatic_collection_pause_count > 0);
+    gcstate->automatic_collection_pause_count--;
 }
 
 /* Public API to invoke gc.collect() from C */
@@ -2003,10 +1999,9 @@ _PyObject_GC_Link(PyObject *op)
     gc->_gc_prev = 0;
     gcstate->generations[0].count++; /* number of allocated GC objects */
     if (gcstate->generations[0].count > gcstate->generations[0].threshold &&
-        gcstate->enabled &&
-        !_Py_atomic_load_int_relaxed(
-            &gcstate->automatic_collection_pause_count) &&
         gcstate->generations[0].threshold &&
+        gcstate->enabled &&
+        !gcstate->automatic_collection_pause_count &&
         !_Py_atomic_load_int_relaxed(&gcstate->collecting) &&
         !_PyErr_Occurred(tstate))
     {
@@ -2019,8 +2014,7 @@ _Py_RunGC(PyThreadState *tstate)
 {
     GCState *gcstate = get_gc_state();
     if (!gcstate->enabled ||
-        _Py_atomic_load_int_relaxed(
-            &gcstate->automatic_collection_pause_count)) {
+        gcstate->automatic_collection_pause_count) {
         return;
     }
     gc_collect_main(tstate, GENERATION_AUTO, _Py_GC_REASON_HEAP);

Python/gc.c

@@ -2001,10 +2001,12 @@ gc_should_collect(GCState *gcstate)
 {
     int count = _Py_atomic_load_int_relaxed(&gcstate->young.count);
     int threshold = gcstate->young.threshold;
-    int gc_enabled = _Py_atomic_load_int_relaxed(&gcstate->enabled);
-    int pause_count = _Py_atomic_load_int_relaxed(
-        &gcstate->automatic_collection_pause_count);
-    if (count <= threshold || threshold == 0 || !gc_enabled || pause_count) {
+    if (count <= threshold || threshold == 0) {
+        return false;
+    }
+    if (!_Py_atomic_load_int_relaxed(&gcstate->enabled) ||
+        _Py_atomic_load_int_relaxed(
+            &gcstate->automatic_collection_pause_count)) {
         return false;
     }
     if (gcstate->old[0].threshold == 0) {
@@ -2073,8 +2075,7 @@ gc_collect_internal(PyInterpreterState *interp,
 {
     _PyEval_StopTheWorld(interp);
 
-    // A concurrent deferral may begin after this collection has emitted its
-    // start notification, but it must take effect before any heap traversal.
+    // Close the race with a deferral that started before the world stopped.
     if (state->reason == _Py_GC_REASON_HEAP &&
         _Py_atomic_load_int(
             &state->gcstate->automatic_collection_pause_count)) {
@@ -2246,8 +2247,6 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
         s->object_stats.object_visits = 0;
     }
 #endif
-    GC_STAT_ADD(generation, collections, 1);
-
     if (reason != _Py_GC_REASON_SHUTDOWN) {
         invoke_gc_callback(tstate, "start", generation, 0, 0, 0, 0.0);
     }
@@ -2282,6 +2281,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
         _Py_atomic_store_int(&gcstate->collecting, 0);
         return 0;
     }
+    GC_STAT_ADD(generation, collections, 1);
 
     m = state.collected;
     n = state.uncollectable;

Python/gc_free_threading.c

@@ -1765,26 +1765,28 @@ static PyObject *
 read_object(RFILE *p)
 {
     PyObject *v;
+    int from_memory = p->ptr && p->end;
     if (PyErr_Occurred()) {
         fprintf(stderr, "XXX readobject called with exception set\n");
         return NULL;
     }
-    if (p->ptr && p->end) {
+    if (from_memory) {
         if (PySys_Audit("marshal.loads", "y#", p->ptr, (Py_ssize_t)(p->end - p->ptr)) < 0) {
             return NULL;
         }
-        PyThreadState *tstate = _PyThreadState_GET();
-        _PyGC_DeferAutomaticCollection(tstate);
-        v = r_object(p);
-        _PyGC_ResumeAutomaticCollection(tstate);
     } else if (p->fp || p->readable) {
         if (PySys_Audit("marshal.load", NULL) < 0) {
             return NULL;
         }
-        v = r_object(p);
     }
-    else {
-        v = r_object(p);
+    PyThreadState *tstate;
+    if (from_memory) {
+        tstate = _PyThreadState_GET();
+        _PyGC_DeferAutomaticCollection(tstate);
+    }
+    v = r_object(p);
+    if (from_memory) {
+        _PyGC_ResumeAutomaticCollection(tstate);
     }
     if (v == NULL && !PyErr_Occurred())
         PyErr_SetString(PyExc_TypeError, "NULL object in marshal data for object");