-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
GH-115776: Static object are immortal, so mark them as such. #117673
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
Conversation
markshannon
commented
Apr 9, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Inline values array into the object #115776
Should this be backported to 3.12? Is it an actual bug / would we expect any impact on third-party extensions because of this? |
It only surfaced as a result of #116115, so maybe not. |
@eduardo-elizondo do you recall why the refcount was set to 1 for extension code? |
Fixes the issue with the latest CPython master. There's a comment above the definitions that also needs adapting. |
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.
This seems reasonable. An initial refcount of 1 makes the object effectively immortal without any of the benefits.
The only potential problem is if an extension is expecting their static type/object to have a normal refcount (starting at 1). However, that probably isn't worth worrying about.
The only thing I'd recommend is that you update the comment at line 118 to reflect the change. If you know of any other similar comments elsewhere in the code it would be worth updating those too.
My gut tells me a changelog entry should suffice there. Extension authors (especially ones who care about this detail) are used to this sort of thing. |
@markshannon This change is totally valid - the reason it was not done in the first place is to avoid incompatibility with extension code. There could be asserts/tests on exact refcount values and this change would have broken those builds/tests. You can even see it in the immoral objects PR where I had to change _testembed.c to modify the There's an argument on whether or not having any exact refcounts checks are correct, but I didn't want to challenge it at the time. Thus, to avoid any problems with extension code and reduce the surface area of impact, I localized it to just affect the CPython build. That said, happy to see that you are now cleaning this up! |
For what it's worth, this change did break one numpy test, see the PR linked above. That said, I think this is a good change and it was trivial to update the test by adding a python version check. |
It broke vim tests too vim/vim#15838 (comment) |