GH-124715: Move trashcan mechanism into `Py_Dealloc` #132280

markshannon · 2025-04-08T16:36:58Z

Issue: Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715

nascheme · 2025-04-08T19:54:05Z

This is a good idea, IMHO. I had a similar PR but Mark's is more elegant than mine.

nascheme · 2025-04-08T19:55:10Z

Objects/object.c

    _PyObject_ASSERT(op, Py_REFCNT(op) == 0);
 #ifdef Py_GIL_DISABLED
    op->ob_tid = (uintptr_t)tstate->delete_later;
 #else
-    _PyGCHead_SET_PREV(_Py_AS_GC(op), (PyGC_Head*)tstate->delete_later);
+    *((PyObject**)op) = tstate->delete_later;


Line 2915 is a little obscure. I think a comment explaining that ob_refcnt is being used to create the queue would be helpful.

bedevere-bot · 2025-04-09T12:28:52Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 8aa20f2 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F132280%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

markshannon · 2025-04-09T13:55:49Z

Initial performance numbers are a bit down. 0.2% - 1.6% slower.

I'm testing another branch that adds PyThreadState * as a parameter to _PyDealloc, which might reduce the impact.

markshannon · 2025-04-10T08:58:39Z

With faster recursion checks, performance is mixed but no worse overall:

Linux ARM: 0.4% slower
Linux x86: 1.5% slower
Windows x86: 0.9% faster
Mac (ARM): 3.8% faster

Although the 1.5% slowdown on linux x86 is concerning.

I also tried passing in the thread state as a parameter to _Py_Dealloc which is a much larger change. Results were a bit worse than this PR:

Linux ARM: no change
Linux x86: 1.3% slower
Windows x86: 1.3% slower
Mac (ARM): 3.9% faster

iritkatriel · 2025-04-10T10:42:48Z

Maybe perf will be better with just one additional if for a decref that is not a container. So you have an implementation of decref for containers, and do one if to decide whether to apply it or stay with the current one.

markshannon · 2025-04-11T14:29:45Z

Maybe perf will be better with just one additional if for a decref that is not a container. So you have an implementation of decref for containers, and do one if to decide whether to apply it or stay with the current one.

Do you mean a test in Py_DECREF or in Py_Dealloc?
We really want to avoid adding more branches to Py_DECREF as Py_DECREF is inlined in so many places.

Adding a branch in Py_Dealloc would save the stack checks, but those are cheap. Getting the "margin" is a load, subtract and shift.

Testing for collections is more expensive. PyObject_IS_GC does three loads, two of which are dependent on the first.

iritkatriel · 2025-04-11T15:18:27Z

Maybe perf will be better with just one additional if for a decref that is not a container. So you have an implementation of decref for containers, and do one if to decide whether to apply it or stay with the current one.

Do you mean a test in Py_DECREF or in Py_Dealloc?

Sorry, yeah, dealloc. Maybe not worth it.

vstinner

The overall design LGTM.

I added a few comments.

vstinner · 2025-04-15T13:51:26Z

Objects/typeobject.c

@@ -2656,45 +2655,6 @@ subtype_dealloc(PyObject *self)
        _Py_DECREF_TYPE(type);
    }



You can now remove this empty line.

vstinner · 2025-04-15T13:55:28Z

Include/pythonrun.h

@@ -26,17 +26,25 @@ PyAPI_DATA(int) (*PyOS_InputHook)(void);
 * apart. In practice, that means it must be larger than the C
 * stack consumption of PyEval_EvalDefault */
 #if defined(_Py_ADDRESS_SANITIZER) || defined(_Py_THREAD_SANITIZER)
-#  define PYOS_STACK_MARGIN 4096
+#  define PYOS_LOG_STACK_MARGIN 12


You may rename this constant to PYOS_LOG2_STACK_MARGIN. It took me a while to understand that it was the log(n)/log(2) function :-) Or just add a a comment explaining what "LOG" stands for here.

vstinner · 2025-04-15T13:58:05Z

Objects/capsule.c

-    PyObject_GC_UnTrack(op);
+    if (_PyObject_GC_IS_TRACKED(op)) {
+        PyObject_GC_UnTrack(op);
+    }


Is this change needed? PyObject_GC_UnTrack() already checks if the object is tracked or not, no?

vstinner · 2025-04-15T13:59:32Z

Misc/NEWS.d/next/Core_and_Builtins/2025-04-08-17-48-11.gh-issue-124715.xxzQoD.rst

+Prevents against stack overflows when calling Py_DECREF. Third-party
+extension objects no longer need to use the "trashcan" mechanism, as
+protection is now built into the ``Py_DECREF`` macro.


Suggested change

Prevents against stack overflows when calling Py_DECREF. Third-party

extension objects no longer need to use the "trashcan" mechanism, as

protection is now built into the ``Py_DECREF`` macro.

Prevents against stack overflows when calling :c:func:`Py_DECREF`. Third-party

extension objects no longer need to use the "trashcan" mechanism, as

protection is now built into the :c:func:`Py_DECREF` function.

vstinner · 2025-04-15T14:02:27Z

Objects/object.c

@@ -2933,7 +2935,9 @@ _PyTrash_thread_destroy_chain(PyThreadState *tstate)
        op->ob_tid = 0;
        _Py_atomic_store_ssize_relaxed(&op->ob_ref_shared, _Py_REF_MERGED);
 #else
-        tstate->delete_later = (PyObject*) _PyGCHead_PREV(_Py_AS_GC(op));
+        uintptr_t refcnt = *((uintptr_t*)op);


The -1 operation is a bit magic, I suggest adding this comment:

Suggested change

uintptr_t refcnt = *((uintptr_t*)op);

/* Get the delete_later pointer from the refcnt field.

* See _PyTrash_thread_deposit_object(). */

uintptr_t refcnt = *((uintptr_t*)op);

vstinner · 2025-04-15T14:02:49Z

Objects/object.c

    _PyObject_ASSERT(op, Py_REFCNT(op) == 0);
 #ifdef Py_GIL_DISABLED
    op->ob_tid = (uintptr_t)tstate->delete_later;
 #else
-    _PyGCHead_SET_PREV(_Py_AS_GC(op), (PyGC_Head*)tstate->delete_later);
+    /* Store the pointer in the refcnt field.


Suggested change

/* Store the pointer in the refcnt field.

/* Store the delete_later pointer in the refcnt field.

Move trashcan mechanism into Py_Dealloc

a7c0082

bedevere-app bot mentioned this pull request Apr 8, 2025

Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715

Open

markshannon force-pushed the trashcan-in-dealloc branch from 47c2439 to a7c0082 Compare April 8, 2025 16:43

markshannon added 3 commits April 8, 2025 17:48

Add news

2cb0a82

Avoid messing with different refcount fields

2d079a1

Remove now-unused label

6ec46fa

nascheme reviewed Apr 8, 2025

View reviewed changes

This was referenced Apr 8, 2025

GH-124715: trashcan protection for all GC objects #124716

Closed

bpo-44881: Integrate GC untrack into trashcan begin. #27718

Closed

markshannon added 3 commits April 9, 2025 10:29

Make sure that the GC never sees objects with refcount of zero

235490e

Update comments

c805603

rst not md

8aa20f2

markshannon marked this pull request as ready for review April 9, 2025 12:28

markshannon requested review from iritkatriel, ericsnowcurrently, methane, rhettinger and 1st1 as code owners April 9, 2025 12:28

bedevere-app bot added the awaiting core review label Apr 9, 2025

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 9, 2025

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 9, 2025

Speedup recursion checks

3514aa1

vstinner approved these changes Apr 15, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Apr 15, 2025

rhettinger removed their request for review April 16, 2025 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-124715: Move trashcan mechanism into `Py_Dealloc` #132280

GH-124715: Move trashcan mechanism into `Py_Dealloc` #132280

markshannon commented Apr 8, 2025 •

edited by bedevere-app bot

Loading

nascheme commented Apr 8, 2025

nascheme Apr 8, 2025

bedevere-bot commented Apr 9, 2025

markshannon commented Apr 9, 2025

markshannon commented Apr 10, 2025

iritkatriel commented Apr 10, 2025

markshannon commented Apr 11, 2025

iritkatriel commented Apr 11, 2025

vstinner left a comment

vstinner Apr 15, 2025

vstinner Apr 15, 2025

vstinner Apr 15, 2025

vstinner Apr 15, 2025

vstinner Apr 15, 2025

vstinner Apr 15, 2025

		@@ -2656,45 +2655,6 @@ subtype_dealloc(PyObject *self)
		_Py_DECREF_TYPE(type);
		}

	/* Store the pointer in the refcnt field.
	/* Store the delete_later pointer in the refcnt field.

GH-124715: Move trashcan mechanism into Py_Dealloc #132280

Are you sure you want to change the base?

GH-124715: Move trashcan mechanism into Py_Dealloc #132280

Conversation

markshannon commented Apr 8, 2025 • edited by bedevere-app bot Loading

nascheme commented Apr 8, 2025

nascheme Apr 8, 2025

Choose a reason for hiding this comment

bedevere-bot commented Apr 9, 2025

markshannon commented Apr 9, 2025

markshannon commented Apr 10, 2025

iritkatriel commented Apr 10, 2025

markshannon commented Apr 11, 2025

iritkatriel commented Apr 11, 2025

vstinner left a comment

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

vstinner Apr 15, 2025

Choose a reason for hiding this comment

GH-124715: Move trashcan mechanism into `Py_Dealloc` #132280

GH-124715: Move trashcan mechanism into `Py_Dealloc` #132280

markshannon commented Apr 8, 2025 •

edited by bedevere-app bot

Loading