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_REFCNT() as opaque function call by vstinner · Pull Request #112747 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Dec 5, 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, the Py_REFCNT() function is now implemented as an opaque function call.

  • 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.

All reactions
@bedevere-app bedevere-app Bot added the awaiting core review label Dec 5, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Dec 5, 2023
--disable-gil builds are not compatible with the limited API #111506
Closed
@vstinner
pythongh-111506: Implement Py_REFCNT() as opaque function call
4fd7ed6
In the limited C API version 3.13, the Py_REFCNT() function is now
implemented as an opaque function call.
@vstinner
vstinner force-pushed the py_getrefcnt branch from d52b1e9 to 4fd7ed6 Compare December 5, 2023 09:41
@vstinner
Update the stable ABI
21a3cff
@vstinner
vstinner requested review from a team and encukou as code owners December 5, 2023 10:07
@vstinner vstinner mentioned this pull request Dec 5, 2023
gh-112535: Update _Py_ThreadId() to support PowerPC, IBM Z, etc #112553
Closed
@vstinner

vstinner commented Dec 5, 2023

Copy link
Copy Markdown
Member Author

See also discussion #112553

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Dec 5, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

With this change, all functions reading/setting a Python object reference count go through opaque functions calls in the limited C API version 3.13 or newer:

  • Py_INCREF(), Py_XINCREF()
  • Py_NewRef(), Py_XNewRef()
  • Py_DECREF(), Py_XDECREF()
  • Py_CLEAR()
  • Py_REFCNT()
  • Py_SET_REFCNT()

So implementation details such as immortal objects (PEP 638) and free threading (PEP 703) "deferred reference counting" are no longer leaked at the ABI level.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Dec 5, 2023

Copy link
Copy Markdown
Member

I don't think Py_REFCNT should be added to the limited API.
What are the use cases for it?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Dec 5, 2023

Copy link
Copy Markdown
Member Author

I don't think Py_REFCNT should be added to the limited API.

The Py_REFCNT() macro (now it's a static inline function) is part of the limited C API since Python 3.2 (PEP 384).

This change only fix its implementation to keep the backward compatibility at the API level. Currently, . You can build existing C extensions without having to change their C code. You just have to target the limited C API version 3.13 if you want to support the free threading build.

Currently, building Modules/xxlimited.c with the limited C API of a free threading build fails with:

./Include/object.h:297:22: error: implicit declaration of function '_Py_atomic_load_uint32_relaxed' [-Werror=implicit-function-declaration]
./Include/object.h:301:25: error: implicit declaration of function '_Py_atomic_load_ssize_relaxed' [-Werror=implicit-function-declaration]

You can test locally with this change:

diff --git a/Modules/xxlimited.c b/Modules/xxlimited.c
index 19f61216255..6d1ced8fb4a 100644
--- a/Modules/xxlimited.c
+++ b/Modules/xxlimited.c
@@ -62,14 +62,7 @@
           pass
    */
 
-#ifndef _MSC_VER
-#include "pyconfig.h"   // Py_GIL_DISABLED
-#endif
-
-#ifndef Py_GIL_DISABLED
-// Need limited C API version 3.12 for Py_MOD_PER_INTERPRETER_GIL_SUPPORTED
-#define Py_LIMITED_API 0x030c0000
-#endif
+#define Py_LIMITED_API 0x030d0000
 
 #include "Python.h"
 #include <string.h>
All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Dec 5, 2023

Copy link
Copy Markdown
Member

Oh, I see. Py_REFCNT is not included in the limited API manifest, which is a bug (but a very low priority one I guess).

So, we're removing ob_refcnt (which is also specified in PEP 384), but keeping Py_REFCNT?

Is there a reason to name the function _Py_GetRefcnt, rather than keep the name people should use -- Py_REFCNT?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Dec 5, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

So, we're removing ob_refcnt (which is also specified in PEP 384), but keeping Py_REFCNT?

This PR doesn't change PyObject.ob_refcnt. We might need to change it to fully implement free threading, but it's wider than the scope of this small change which is backward compatible (EDIT: compatible!).

Is there a reason to name the function _Py_GetRefcnt, rather than keep the name people should use -- Py_REFCNT?

I don't think that a regular function and a static inline functions can have the same name. With macros, it's possible to reuse the same name, but I don't want to use a macro here.

Py_INCREF() and Py_SET_REFCNT() have a similiar design: call an opaque function which has a different name and is only exposed as ABI only in the stable ABI.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Dec 6, 2023

Copy link
Copy Markdown
Member

I don't think that a regular function and a static inline functions can have the same name.

Please discuss at capi-workgroup/api-evolution#18, which has an example of how to do this.

With macros, it's possible to reuse the same name, but I don't want to use a macro here.

Not even the simple alias macro, #define Py_Foo _Py_Foo_impl?

Py_INCREF() and Py_SET_REFCNT() have a similiar design: call an opaque function which has a different name and is only exposed as ABI only in the stable ABI.

Yeah, that's the bug I was talking about. It's guideline issue 18 again: “All functions should be exported as proper symbols, so they are usable without a C preprocessor/parser.”

With this PR, Py_REFCNT is only usable in C. I've long thought that this is bad, but now I think that it should be more than personal preference, so, unlike in October, I'll block PRs on it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Dec 6, 2023
Avoid macros and static inline functions capi-workgroup/api-evolution#18
Closed
@vstinner

vstinner commented Dec 20, 2023

Copy link
Copy Markdown
Member Author

Apparently, there is a discussion/disagreement on the API/name in the ABI. I prefer to close the PR until a decision is taken.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner closed this Dec 20, 2023
@vstinner
vstinner deleted the py_getrefcnt branch January 14, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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

awaiting core review

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.

2 participants

@vstinner @encukou
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.