| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Also use it in _bz2.
There was a problem hiding this comment.
I think we should be smarter here:
(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).
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Yeah, I found a few other types that don't need this. But as I said in the PR description:
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). |
Sorry, something went wrong.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
|
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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
…ythonGH-138343) Add `_PyObject_VisitType` in place of `tp_traverse` functions that only visit the object's type.
Also, I want to work on this. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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 :-( |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.