WebSafe 3.7github.com
|
|
🏠
Skip to content

GH-124715: Move trashcan mechanism into Py_Dealloc#132280

Merged
markshannon merged 9 commits intopython:mainfrom
faster-cpython:trashcan-in-dealloc
Apr 30, 2025
Merged

GH-124715: Move trashcan mechanism into Py_Dealloc#132280
markshannon merged 9 commits intopython:mainfrom
faster-cpython:trashcan-in-dealloc

Conversation

@markshannon
Copy link
Member

@markshannon markshannon commented Apr 8, 2025

@nascheme
Copy link
Member

nascheme commented Apr 8, 2025

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

@markshannon markshannon marked this pull request as ready for review April 9, 2025 12:28
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 9, 2025
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 9, 2025
@markshannon
Copy link
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.

@markshannon
Copy link
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
@iritkatriel
Copy link
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.

@markshannon
Copy link
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.

@iritkatriel
Copy link
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.

Copy link
Member

@vstinner vstinner left a 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.

@rhettinger rhettinger removed their request for review April 16, 2025 02:26
@markshannon markshannon merged commit 44e4c47 into python:main Apr 30, 2025
48 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ 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', []]]]] != []
@vstinner
Copy link
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
  (...)
vstinner added a commit to vstinner/cpython that referenced this pull request Apr 30, 2025
Replace _PyObject_GC_UNTRACK() with PyObject_GC_UnTrack() to not fail
if the method was already untracked.
@vstinner
Copy link
Member

I wrote #133199 to fix test_functools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

Comments