Skip to content

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Sep 1, 2025

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.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

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

@picnixz
Copy link
Member

picnixz commented Sep 1, 2025

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.

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

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

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?

Co-authored-by: Adam Turner <[email protected]>
@rhettinger rhettinger removed their request for review September 1, 2025 15:44
@ZeroIntensity ZeroIntensity enabled auto-merge (squash) September 1, 2025 16:12
@ZeroIntensity ZeroIntensity merged commit 4f6ecd1 into python:main Sep 1, 2025
43 checks passed
@ZeroIntensity ZeroIntensity deleted the visit-type branch September 1, 2025 16:20
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Sep 3, 2025

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?

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

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.

5 participants