GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-124715: Move trashcan mechanism into `Py_Dealloc` by markshannon · Pull Request #132280 · python/cpython

Conversation

@markshannon

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@markshannon
Move trashcan mechanism into Py_Dealloc
a7c0082
@bedevere-app bedevere-app Bot mentioned this pull request Apr 8, 2025
Not all types have "trashcan" protection and tp_dealloc can overflow stack #124715
Closed
@markshannon
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
@markshannon
Add news
2cb0a82
@markshannon
Avoid messing with different refcount fields
2d079a1
@markshannon
Remove now-unused label
6ec46fa
@nascheme

nascheme commented Apr 8, 2025

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nascheme
nascheme reviewed Apr 8, 2025
View reviewed changes
Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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
@markshannon
Make sure that the GC never sees objects with refcount of zero
235490e
@markshannon
Update comments
c805603
@markshannon
rst not md
8aa20f2
@markshannon
markshannon marked this pull request as ready for review April 9, 2025 12:28
@markshannon
markshannon requested review from 1st1, ericsnowcurrently, iritkatriel, methane and rhettinger as code owners April 9, 2025 12:28
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 9, 2025
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 9, 2025
@bedevere-bot

bedevere-bot commented Apr 9, 2025

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

markshannon commented Apr 9, 2025

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Speedup recursion checks
3514aa1
@markshannon

markshannon commented Apr 10, 2025

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Apr 10, 2025

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Apr 11, 2025

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Apr 11, 2025

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner approved these changes Apr 15, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall design LGTM.

I added a few comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/pythonrun.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/capsule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 15, 2025
@rhettinger
rhettinger removed their request for review April 16, 2025 02:26
@markshannon
Address review comments
e2e73df
iritkatriel
iritkatriel approved these changes Apr 30, 2025
View reviewed changes
@markshannon
markshannon merged commit 44e4c47 into python:main Apr 30, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 30, 2025
@bedevere-bot

bedevere-bot commented Apr 30, 2025

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL8 3.x (tier-3) has failed when building commit 44e4c47.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/509/builds/9127) and take a look at the build logs.
  4. Check if the failure is related to this commit (44e4c47) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/509/builds/9127

Failed tests:

  • test_functools

Failed subtests:

  • test_async_global_awaited_by - test.test_external_inspection.TestGetStackTrace.test_async_global_awaited_by

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel8-s390x/build/Lib/test/test_external_inspection.py", line 531, in test_async_global_awaited_by
    self.assertEqual([[['echo_client_spam'], 'echo client spam', [[['main'], 'Task-1', []]]]], entries[-1][1])
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: [[['echo_client_spam'], 'echo client spam', [[['main'], 'Task-1', []]]]] != []
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 30, 2025

Copy link
Copy Markdown
Member

Hi! The buildbot s390x RHEL8 3.x (tier-3) has failed when building commit 44e4c47.

test_recursive_pickle (test.test_functools.TestPartialPy.test_recursive_pickle) ...

Objects/classobject.c:247: _PyObject_GC_UNTRACK: Assertion "_PyObject_GC_IS_TRACKED(((PyObject*)(op)))" failed: object not tracked by the garbage collector
Enable tracemalloc to get the memory block allocation traceback

object address  : 0x3ffae27c8f0
object refcount : 0
object type     : 0x1669768
object type name: method
object repr     : <refcnt 0 at 0x3ffae27c8f0>

Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized

Current thread 0x000003ffb6177270 [python] (most recent call first):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel8-s390x/build/Lib/test/test_functools.py", line 420 in test_recursive_pickle
  (...)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added a commit to vstinner/cpython that referenced this pull request Apr 30, 2025
@vstinner
pythongh-132280: Fix method_dealloc(): use PyObject_GC_UnTrack()
d09167e
Replace _PyObject_GC_UNTRACK() with PyObject_GC_UnTrack() to not fail
if the method was already untracked.
@vstinner

vstinner commented Apr 30, 2025

Copy link
Copy Markdown
Member

I wrote #133199 to fix test_functools.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Apr 30, 2025
gh-124715: Fix method_dealloc(): use PyObject_GC_UnTrack() #133199
Merged
@colesbury colesbury mentioned this pull request Jun 16, 2025
test_weakref.test_threaded_weak_key_dict_deepcopy crash: merged objects should have ob_tid == 0 #135106
Closed
@encukou encukou mentioned this pull request Jan 12, 2026
Undocumented 3.13 C API #118915
Open
61 of 78 tasks
mandolaerik added a commit to mandolaerik/cpython that referenced this pull request Jan 23, 2026
@mandolaerik
Revert "pythonGH-124715: Move trashcan mechanism into Py_Dealloc (p…
236e1bd
…ythonGH-132280)"

This reverts commit 44e4c47.
@bhuvi27 bhuvi27 mentioned this pull request Jun 17, 2026
gh-149146: Add dealloc-depth counter fallback to trashcan trigger #151595
Closed
@markshannon markshannon mentioned this pull request Jul 29, 2026
deeply nested itertools objects segfault #58218
Closed
This was referenced Aug 25, 2026
PyRefTracer_DESTROY is not called for objects deferred by the trashcan mechanism #156371
Closed
gh-156371: Fix missing PyRefTracer_DESTROY events for trashcan objects #156372
Merged
@aeroyorch aeroyorch mentioned this pull request Oct 2, 2026
tp_dealloc example may need the trashcan for GC container types #158619
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nascheme nascheme nascheme left review comments
@vstinner vstinner vstinner approved these changes
@iritkatriel iritkatriel iritkatriel approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@methane methane Awaiting requested review from methane methane is a code owner
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@markshannon @nascheme @bedevere-bot @iritkatriel @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.