Skip to content
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-103906: Remove immortal refcounting in compile/marshal.c #103922

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Apr 27, 2023

@corona10 corona10 marked this pull request as ready for review April 27, 2023 13:39
@corona10
Copy link
Member Author

@brandtbucher Not sure worth to do it at compile.c as much as an interpreter but it looks possible.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for 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.

Python/compile.c Outdated Show resolved Hide resolved
@corona10 corona10 requested a review from tiran as a code owner April 27, 2023 14:15
@iritkatriel
Copy link
Member

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?

@corona10 corona10 changed the title gh-103906: Remove immortal refcounting in compile.c gh-103906: Remove immortal refcounting in compile/marshal.c Apr 27, 2023
@corona10
Copy link
Member Author

Should we have Py_New_None/Py_New_Elipsis/etc macros instead, so that the code is clear about this?

I think that it will be the proper way and easy to track.

@JelleZijlstra
Copy link
Member

Should we have Py_New_None/Py_New_Elipsis/etc macros instead, so that the code is clear about this?

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.

@corona10
Copy link
Member Author

@vstinner What do you think about this agenda?

@iritkatriel
Copy link
Member

Should we have Py_New_None/Py_New_Elipsis/etc macros instead, so that the code is clear about this?

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.

True. We would need a test mode where they are not immortal and the macros actually change the refcount.

@corona10
Copy link
Member Author

corona10 commented Apr 27, 2023

True. We would need a test mode where they are not immortal and the macros actually change the refcount.

I am not sure that I understand your intention correctly, so do we need to introduce the following configuration things?
./configure --emulate-ref-count
So if such a configuration is enabled, Py_New_None will execute ref counting API?

@JelleZijlstra
Copy link
Member

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.

@corona10
Copy link
Member Author

corona10 commented Apr 27, 2023

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 --with-experimental-isolated-subinterpreters flag:
#90988

@corona10
Copy link
Member Author

corona10 commented Apr 27, 2023

My suggestion:
How about adding test cases(whitelist) for immortal objects from the C API side rather than adding something like emulation mode.
If we add the test code that can occur weird refcount, under the immortal object condition, the test will be passed, but if we disable the immortal object condition from a specific object, the test will be failed, and we can easily track what should be fixed.

@iritkatriel
Copy link
Member

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.

@vstinner
Copy link
Member

vstinner commented May 4, 2023

@vstinner What do you think about this agenda?

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

@brandtbucher brandtbucher removed their request for review May 10, 2023 22:01
@corona10
Copy link
Member Author

corona10 commented Jun 5, 2023

It looks like #105195 will be merged soon.
Only applying this PR for Python 3.13 will not be harmful if the documentation exists.

I merge this PR, please let me know if the PR occurs any problem.

@corona10 corona10 merged commit 058b960 into python:main Jun 5, 2023
@corona10 corona10 deleted the gh-103906 branch June 5, 2023 13:38
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