GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103906: Remove immortal refcounting in compile/marshal.c by corona10 · Pull Request #103922 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Apr 27, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Avoid reference counting operations on None, True, False in the interpreter #103906

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@corona10
pythongh-103906: Remove immortal refcounting in compile.c
8cdd24d
@bedevere-bot bedevere-bot mentioned this pull request Apr 27, 2023
Avoid reference counting operations on None, True, False in the interpreter #103906
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Apr 27, 2023
@corona10
Remove refcounting for Py_None
6d0dd68
@corona10 corona10 added the skip news label Apr 27, 2023
@corona10
corona10 marked this pull request as ready for review April 27, 2023 13:39
@corona10
corona10 requested review from iritkatriel and markshannon as code owners April 27, 2023 13:39
@corona10
corona10 requested a review from brandtbucher April 27, 2023 13:39
@corona10

corona10 commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed Apr 27, 2023
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

corona10 and others added 2 commits April 27, 2023 23:10
@corona10 @JelleZijlstra
Update Python/compile.c
57d9b5f
Co-authored-by: Jelle Zijlstra <[email protected]>
@corona10
Remove more cases
414eae5
@corona10
corona10 requested a review from tiran as a code owner April 27, 2023 14:15
@iritkatriel

iritkatriel commented Apr 27, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra approved these changes Apr 27, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 27, 2023
@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

corona10 commented Apr 27, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 27, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

@vstinner What do you think about this agenda?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Apr 27, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Apr 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 27, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Apr 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Apr 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Apr 27, 2023

Copy link
Copy Markdown
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.

hauntsaninja reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10 corona10 mentioned this pull request Apr 28, 2023
gh-84436 Add integration C API tests for immortal objects #103962
Merged
@vstinner

vstinner commented May 4, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher
brandtbucher removed their request for review May 10, 2023 22:01
@corona10

corona10 commented Jun 5, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10
corona10 merged commit 058b960 into python:main Jun 5, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 5, 2023
@corona10
corona10 deleted the gh-103906 branch June 5, 2023 13:38
@ashm-dev ashm-dev mentioned this pull request Oct 12, 2026
Avoid redundant reference counting of known Py_None values #159226
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@corona10 @iritkatriel @JelleZijlstra @vstinner @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.