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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-115776: Embed the values array into the object, for "normal" Python objects. #116115
GH-115776: Embed the values array into the object, for "normal" Python objects. #116115
Changes from all commits
d5085db
0d88de4
d922903
c03e6cb
621ea3b
67daab5
34c7171
c237e79
f80befa
a0c11e4
bc1ebc8
0dc68a4
084519c
3744f32
75ee5a0
162764c
82ece1b
6a762ed
9dbc8dd
4a1f7b7
09121f9
c384b05
ee5cf2a
005b42b
48d849e
682217c
0ff1709
895a944
1b4302a
ecd4204
c05d01d
d441d7b
3395bc2
ccceffb
7700177
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I now hit this assertion in Cython modules. The (static) extension type that we use for generators has
tp_alloc = PyType_GenericAlloc
and calls it intp_new()
:Is this assertion simply wrong, or is there anything I can do to avoid it? The "immortal objects" API doesn't seem to be intended for public use.
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.
#19474 sets the reference count to 1 for statically allocated objects unless
Py_BUILD_CORE
is defined.As a workaround, I'd suggest making these classes immortal and making them immutable if you can.
That does seem to be the case. I don't know why. Want to open an issue for that?
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.
Is there a reason why this new assertion exists? The previous
if
condition came from a conservative change that intended to avoid refcounting static types but wanted to make sure (heap) type objects are correctly kept alive as long as their objects. Now the code requires all non-heap types to be declared immortal. That seems a rather heavy change in requirements.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.
All non-heap types are immortal. Requiring that they are declared as such seems reasonable, and is probably necessary for memory safety.
I don't think you should need to do so explicitly though,
PyObject_HEAD_INIT
should do it.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.
Does #117673 fix the problem for you?
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.