GeezerVison is showing github.com — gh-111506: Implement Py_REFCNT() as opaque function call by vstinner · Pull Request #112747 · python/cpython
Conversation
vstinner
commented
Dec 5, 2023
•
edited by bedevere-app
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
In the limited C API version 3.13, the Py_REFCNT() function is now implemented as an opaque function call.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
In the limited C API version 3.13, the Py_REFCNT() function is now
implemented as an opaque function call.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Dec 5, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I don't think Py_REFCNT should be added to the limited API.
What are the use cases for it?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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>
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Dec 5, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Apparently, there is a discussion/disagreement on the API/name in the ABI. I prefer to close the PR until a decision is taken.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
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
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.
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.
|