FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-138342: Use a common utility for visiting an object's type by ZeroIntensity · Pull Request #138343 · python/cpython · GitHub

/ cpython Public

gh-138342: Use a common utility for visiting an object's type - #138343

Merged
ZeroIntensity merged 18 commits into
python:mainfrom
ZeroIntensity:visit-type
Sep 1, 2025
Merged

gh-138342: Use a common utility for visiting an object's type#138343
ZeroIntensity merged 18 commits into
python:mainfrom
ZeroIntensity:visit-type

Conversation

ZeroIntensity commented Sep 1, 2025
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Add _PyObject_VisitType in place of tp_traverse functions that only visit the object's type. This will likely help us catch some bugs through assertions, as we can ensure that this only applies to mutable heap types (and thus are subject to reference cycles). For now, I've kept the assertions relatively relaxed to make it easier to review, but I'll strengthen those assertions and address any bugs in a follow-up.

Sorry about the flood of pings, codeowners.

picnixz left a comment
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we should be smarter here:

  1. Let's first add your utility without it being used.
  2. Remove the traverse & GC support for types that are immutable and empty.
  3. Add the necessary traverse functions for objects whose class is meant to be mutable.

(1) can be achieved independently but (2) and (3) should perhaps be coordinated depending on what we want to backport. I would suggest that we don't change immutability in 3.13 and 3.14 (so we need the traverse functions for those types) but we can do in 3.15 (in which case we won't need this helper).

Comment thread Modules/_bz2module.c
Comment thread Modules/_dbmmodule.c

picnixz commented Sep 1, 2025

Copy link
Copy Markdown
Member

Nevermind, my comment got cross-posted with #116946 (comment). So apparently, it would be safer to not change the immutability of those types for now.

Copy link
Copy Markdown
Member Author

Yeah, I found a few other types that don't need this. But as I said in the PR description:

For now, I've kept the assertions relatively relaxed to make it easier to review, but I'll strengthen those assertions and address any bugs in a follow-up.

This PR is pretty big already, and I would like to avoid sprinkling in behavior changes. Once your work with #116946 is done, we can strengthen the assertions in _PyObject_VisitType (e.g., requiring Py_TPFLAGS_IMMUTABLETYPE not to be set).

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Apart from the changes that I will revert in #138338 are removed from this PR, we can merge this. Do you want to review it?

Comment thread Include/cpython/object.h
Comment thread Modules/_gdbmmodule.c Outdated
Comment thread Modules/md5module.c Outdated
Comment thread Modules/arraymodule.c Outdated
Comment thread Modules/sha1module.c Outdated
Comment thread Modules/sha2module.c Outdated
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
rhettinger removed their request for review September 1, 2025 15:44
ZeroIntensity enabled auto-merge (squash) September 1, 2025 16:12
ZeroIntensity merged commit 4f6ecd1 into python:main Sep 1, 2025
43 checks passed
ZeroIntensity deleted the visit-type branch September 1, 2025 16:20

kumaraditya303 commented Sep 3, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

Most of these are cases where the object actually doesn't needs to have GC at all, they are empty objects with immutable types. Instead of adding this why not just remove GC from them? It will help in performance and reduce gc pressure.

Also in future when improving the assertions, how would you check for empty objects?

Copy link
Copy Markdown
Member Author

Also in future when improving the assertions, how would you check for empty objects?

Something like assert(!PyType_HasFeature(tp, Py_TPFLAGS_IMMUTABLETYPE)). We couldn't add it here because of what you stated -- a lot of these don't actually need the GC. Bénédikt is working on finding those types, but I wouldn't go as far as to say that most types here don't need it.

lkollar pushed a commit to lkollar/cpython that referenced this pull request Sep 9, 2025
…ythonGH-138343)

Add `_PyObject_VisitType` in place of `tp_traverse` functions that only visit the object's type.

Copy link
Copy Markdown
Contributor

Most of these are cases where the object actually doesn't needs to have GC at all, they are empty objects with immutable types. Instead of adding this why not just remove GC from them? It will help in performance and reduce gc pressure.

Also in future when improving the assertions, how would you check for empty objects?

Also, I want to work on this.

vstinner commented Oct 7, 2025

Copy link
Copy Markdown
Member

Unless there is a good reason to add this private function to the public C API, I would prefer to move it to the internal C API: see my PR #139734.

If tomorrow, this function becomes popular and very useful outside CPython, I would suggest to make it public. But for now, I would prefer to keep it in the internal C API.

ZeroIntensity commented Oct 7, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

I kept it in public headers here to avoid adding #include "pycore_object.h" everywhere, so I have no objections to moving it to the internal headers.

vstinner commented Oct 8, 2025
edited
Loading

Copy link
Copy Markdown
Member

I kept it in public headers here to avoid adding #include "pycore_object.h" everywhere, so I have no objections to moving it to the internal headers.

Later, it's easy to make it public: add it again to the public C API as PyObject_VisitType(). But if it stays in the public C API as a private name, users will start using it and we cannot change it later :-(

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL