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-111506: Implement Py_SET_REFCNT() as opaque function in limited C API by vstinner · Pull Request #111508 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Oct 30, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

In the limited C API version 3.13, Py_SET_REFCNT() function is now implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.

  • Issue: --disable-gil builds are not compatible with the limited API #111506

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji colesbury reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
@vstinner
vstinner requested a review from encukou as a code owner October 30, 2023 17:13
@bedevere-app bedevere-app Bot mentioned this pull request Oct 30, 2023
--disable-gil builds are not compatible with the limited API #111506
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 30, 2023
@vstinner
pythongh-111506: Implement Py_SET_REFCNT() as opaque function in limi…
ac0c1dd 
…ted C API

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
@vstinner
vstinner force-pushed the py_setrefcnt branch from 67638c4 to ac0c1dd Compare October 30, 2023 17:23
@vstinner
vstinner requested a review from a team as a code owner October 30, 2023 17:23
@encukou

encukou commented Oct 31, 2023

Copy link
Copy Markdown
Member

Why is this particular function necessary?
I'd rather not commit to supporting setting the reference count to an arbitrary value in the long term.

abi3 makes it possible -- it exposes ob_refcnt directly. But exposing the fields directly was perhaps the biggest flaw of abi3. We now have a chance to revisit it, and tighten the set of supported operations.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Oct 31, 2023

Copy link
Copy Markdown
Member Author

Why is this particular function necessary?

It's needed to be able to remove non-portable assembly code from Include/*.h. I would prefer to keep these header files as simple C code, no inline assembly.

The code was just change for free threading. This change follows Py_INCREF/DECREF which already use an opaque function call in limited C API version 3.12.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Oct 31, 2023

Copy link
Copy Markdown
Member Author

I'd rather not commit to supporting setting the reference count to an arbitrary value in the long term.

I also have concerns about that. But this change is a practical change to make the situation less bad.

I plan to work on a whole PEP to disallow accessing directly PyObject.ob_refcnt and convert more functions to opaque function calls in the limited C API, as Sam describes in the issue.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

@colesbury @serhiy-storchaka @erlend-aasland: Would you mind to review this change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

I'll leave this to @colesbury.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury approved these changes Nov 3, 2023
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

I think this only improves our compatibility story going forward. We have already committed to this functionality as part of the API and ABI. Using an opaque function instead of an inline function does not change this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@encukou

encukou commented Nov 3, 2023

Copy link
Copy Markdown
Member

+1 API-wise -- keeping it abi_only is the way to go.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 20cfab9 into python:main Nov 3, 2023
@vstinner
vstinner deleted the py_setrefcnt branch November 3, 2023 17:18
@bedevere-app bedevere-app Bot removed the awaiting core review label Nov 3, 2023
@vstinner

vstinner commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

Merged, thanks for reviews.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Nov 3, 2023

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Fedora 3.x has failed when building commit 20cfab9.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/223/builds/4734) and take a look at the build logs.
  4. Check if the failure is related to this commit (20cfab9) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/223/builds/4734

Summary of the results of the build (if available):

==

Click to see traceback logs 
remote: Enumerating objects: 17, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  10% (1/10)        
remote: Compressing objects:  20% (2/10)        
remote: Compressing objects:  30% (3/10)        
remote: Compressing objects:  40% (4/10)        
remote: Compressing objects:  50% (5/10)        
remote: Compressing objects:  60% (6/10)        
remote: Compressing objects:  70% (7/10)        
remote: Compressing objects:  80% (8/10)        
remote: Compressing objects:  90% (9/10)        
remote: Compressing objects: 100% (10/10)        
remote: Compressing objects: 100% (10/10), done.        
remote: Total 17 (delta 3), reused 3 (delta 3), pack-reused 4        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '20cfab903db70cf952128bc6b606e3ec4a216498'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 20cfab903d gh-111506: Implement Py_SET_REFCNT() as opaque function in limited C API (#111508)
Switched to and reset branch 'main'

make: *** [Makefile:2065: buildbottest] Error 3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou mentioned this pull request Dec 6, 2023
gh-111506: Implement Py_REFCNT() as opaque function call #112747
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @aisk
pythongh-111506: Implement Py_SET_REFCNT() as opaque function in limi…
f977bc1 
…ted C API (python#111508)

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@vstinner @Glyphack
pythongh-111506: Implement Py_SET_REFCNT() as opaque function in limi…
a640604 
…ted C API (python#111508)

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes
@encukou encukou Awaiting requested review from encukou encukou is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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

@vstinner @encukou @erlend-aasland @bedevere-bot @colesbury
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.