Skip to content

Commit d47b581

Browse files
committed
gh-159437: Skip unnecessary late weakref cleanup
Count the stable unreachable set while preparing callbacks and retain a conservative cleanup flag. Reuse the count only when late weakref clearing has no work, and force fresh cleanup and counting after resurrection analysis. Add a regression for a callback-free weakref observed when clearing a young cycle releases an older finalizer.
1 parent 768f2d6 commit d47b581

3 files changed

Lines changed: 58 additions & 4 deletions

File tree

‎Lib/test/test_gc.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1196,6 +1196,36 @@ def getstats():
11961196

11971197
gc.enable()
11981198

1199+
@requires_gil_enabled("tests the generation-list collector")
1200+
def test_callback_free_weakref_clear_without_pending_callbacks(self):
1201+
"""Clear callback-free weakrefs before tp_clear can expose garbage."""
1202+
was_enabled = gc.isenabled()
1203+
gc.disable()
1204+
self.addCleanup(gc.enable if was_enabled else gc.disable)
1205+
self.addCleanup(gc.collect)
1206+
observed = []
1207+
self.addCleanup(observed.clear)
1208+
1209+
class Observer:
1210+
"""Inspect a young target when its clearing releases this object."""
1211+
1212+
def __del__(self):
1213+
"""Record whether the weakref still exposes the target."""
1214+
observed.append(reference())
1215+
1216+
observer = Observer()
1217+
gc.collect()
1218+
# The older observer does not open the callback path for generation 0.
1219+
# Clearing the target releases it, so the weakref must already be dead.
1220+
target = C1055820(0)
1221+
target.observer = observer
1222+
reference = weakref.ref(target)
1223+
del observer, target
1224+
1225+
self.assertEqual(gc.collect(0), 1)
1226+
self.assertEqual(observed, [None])
1227+
self.assertIsNone(reference())
1228+
11991229
@unittest.skipIf(ContainerNoGC is None,
12001230
'requires ContainerNoGC extension type')
12011231
def test_trash_weakref_clear(self):
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Garbage collection in builds with the GIL now reuses the initial garbage count and skips late weakref cleanup when no weakrefs need clearing and the unreachable set remains valid.

‎Python/gc.c‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -798,16 +798,22 @@ move_legacy_finalizer_reachable(PyGC_Head *finalizers)
798798
* Arguments:
799799
* - `unreachable`: normalized collecting list to inspect.
800800
* - `wrcb_to_call`: initialized list receiving weakrefs whose callbacks are safe to invoke.
801+
* - `collectable_count_out`: receives the number of objects in `unreachable`.
802+
* - `needs_weakref_cleanup_out`: receives whether garbage weakrefs or remaining incoming weakrefs may need cleanup.
801803
*
802804
* Return whether any finalizer is pending or a weakref callback is queued.
803-
* This function does not invoke callbacks.
805+
* This function does not invoke callbacks. The count and cleanup decision
806+
* remain valid only while the unreachable set and its weakrefs cannot change.
804807
*/
805808
Py_NO_INLINE static bool
806-
prepare_callbacks(PyGC_Head *unreachable, PyGC_Head *wrcb_to_call)
809+
prepare_callbacks(PyGC_Head *unreachable, PyGC_Head *wrcb_to_call,
810+
Py_ssize_t *collectable_count_out, bool *needs_weakref_cleanup_out)
807811
{
808812
PyGC_Head *gc;
809813
PyGC_Head *next;
814+
Py_ssize_t count = 0;
810815
bool has_pending_finalizer = false;
816+
bool needs_weakref_cleanup = false;
811817

812818
/* Find all weakrefs with callbacks and move into `wrcb_to_call` if the
813819
* callback needs to be invoked. We make another pass over wrcb_to_call,
@@ -818,11 +824,16 @@ prepare_callbacks(PyGC_Head *unreachable, PyGC_Head *wrcb_to_call)
818824

819825
PyObject *op = FROM_GC(gc);
820826
PyTypeObject *type = Py_TYPE(op);
827+
count++;
821828
if (type->tp_finalize != NULL && !has_pending_finalizer) {
822829
has_pending_finalizer = !_PyGC_FINALIZED(op);
823830
}
824831
next = GC_NEXT(gc);
825832

833+
// Garbage weakrefs need clearing even if their referents are outside
834+
// this collection (bpo-38006).
835+
needs_weakref_cleanup |= PyWeakref_Check(op);
836+
826837
if (! _PyType_SUPPORTS_WEAKREFS(type)) {
827838
continue;
828839
}
@@ -900,8 +911,12 @@ prepare_callbacks(PyGC_Head *unreachable, PyGC_Head *wrcb_to_call)
900911
_PyObject_ASSERT((PyObject *)wr, wrasgc != next);
901912
gc_list_move(wrasgc, wrcb_to_call);
902913
}
914+
// Callback-free weakrefs remain linked until finalization.
915+
needs_weakref_cleanup |= *wrlist != NULL;
903916
}
904917

918+
*collectable_count_out = count;
919+
*needs_weakref_cleanup_out = needs_weakref_cleanup;
905920
return has_pending_finalizer || !gc_list_is_empty(wrcb_to_call);
906921
}
907922

@@ -1632,7 +1647,9 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
16321647

16331648
PyGC_Head wrcb_to_call;
16341649
gc_list_init(&wrcb_to_call);
1635-
bool has_pending_callbacks = prepare_callbacks(&unreachable, &wrcb_to_call);
1650+
Py_ssize_t collectable_count;
1651+
bool needs_weakref_cleanup;
1652+
bool has_pending_callbacks = prepare_callbacks(&unreachable, &wrcb_to_call, &collectable_count, &needs_weakref_cleanup);
16361653
has_pending_callbacks |= needs_resurrection_check;
16371654

16381655
PyGC_Head final_unreachable;
@@ -1641,6 +1658,9 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
16411658
bool resurrection_checked = handle_callbacks(tstate, collectable, &final_unreachable, old, &wrcb_to_call, &stats.collected, needs_resurrection_check);
16421659
if (resurrection_checked) {
16431660
collectable = &final_unreachable;
1661+
// Python code may have changed the garbage set or created weakrefs.
1662+
// Refresh both cleanup and counting.
1663+
needs_weakref_cleanup = true;
16441664
}
16451665
}
16461666
validate_list(old, collecting_clear_unreachable_clear);
@@ -1652,7 +1672,10 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
16521672
* and create new weakrefs. If those weakrefs were not cleared, they
16531673
* could reveal unreachable objects. Callbacks are not executed.
16541674
*/
1655-
stats.collected += clear_weakrefs(collectable);
1675+
if (needs_weakref_cleanup) {
1676+
collectable_count = clear_weakrefs(collectable);
1677+
}
1678+
stats.collected += collectable_count;
16561679

16571680
/* Call tp_clear on objects in the collectable set. This will cause
16581681
* the reference cycles to be broken. It may also cause some objects

0 commit comments

Comments
 (0)