GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||
GeezerVison is showing github.com — gh-103906: Remove immortal refcounting in compile/marshal.c by corona10 · Pull Request #103922 · python/cpython Conversation
|
|
@brandtbucher Not sure worth to do it at compile.c as much as an interpreter but it looks possible. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If you grep for Py_NewRef(Py_Ellipsis) there are a few more in other files, though I doubt it will make a measurable difference in performance.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Jelle Zijlstra <[email protected]>
|
I'm a bit concerned that a change like this would make it hard to reverse the decision to make these objects immortal in the future (it is no longer clear where we need a new reference and where we don't). Should we have Py_New_None/Py_New_Elipsis/etc macros instead, so that the code is clear about this? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I think that it will be the proper way and easy to track. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I'm not sure that will be workable. Since there is no refcount change, there will be no way to enforce that these macros are used correctly, so we're likely to regress very quickly on correct use of those macros. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@vstinner What do you think about this agenda? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
True. We would need a test mode where they are not immortal and the macros actually change the refcount. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I am not sure that I understand your intention correctly, so do we need to introduce the following configuration things? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, we'd need something like that. We'd have to run at least one buildbot under that configuration mode too. I'm honestly not sure it would be worth introducing that much complexity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
One of my concerns about adding such a configuration which determining the C API or macro behavior, as @JelleZijlstra commented it should always be run under at least from the build bot with the configuration enabled. Without the integration from the CI level, such configuration is easily broken and can not guarantee to be built successfully at any time. This is one of my lessons from the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
My suggestion: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
We need a decision on whether we want to put some effort into making the move immortality reversible, or not. The technicalities are not very difficult once that decision is made. I'm not sure who should make the call, but it's not me. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The SC approved https://peps.python.org/pep-0683/ Why would be move backward and remove immortal objects? I'm not used to immortal objects, so right now I'm surprised that some objects require Py_NewRef() whereas others don't. It makes the code less regular and so harder to review for me. But I prefer to stay outside this topic and let others review it :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It looks like #105195 will be merged soon. I merge this PR, please let me know if the PR occurs any problem. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.