gh-133164: Add PyUnstable_Object_IsUniqueReferencedTemporary C API
#133170
Conversation
After pythongh-130704, the interpreter replaces some uses of `LOAD_FAST` with `LOAD_FAST_BORROW` which avoid incref/decrefs by "borrowing" references on the interpreter stack when the bytecode compiler can determine that its safe. This change broke some checks in C API extensions that relied on `Py_REFCNT()` of `1` to determine if it's safe to modify an object in-place. Objects may have a reference count of one, but still be referenced further up the interpreter stack due to borrowing of references. This provides a replacement function for those checks. `PyUnstable_Object_IsUniqueTemporary` is more conservative: it checks that the object has a reference count of one and that it exists as a unique strong reference in the interpreter's stack of temporary variables in the top most frame. See also: * numpy/numpy#28681
Misc/NEWS.d/next/C_API/2025-04-29-19-39-16.gh-issue-133164.W-XTU7.rst
Outdated
Show resolved
Hide resolved
|
@ngoldbaum - I can't add you as a reviewer, but I'd appreciate it if you could take a look at this when you get a chance. |
Doc/c-api/object.rst
Outdated
| temporary object. | ||
|
|
||
| If an object is a unique temporary, it is guaranteed that the reference | ||
| count is ``1`` and it may be safe to modify the object in-place becuase |
There was a problem hiding this comment.
I think "may be safe" doesn't quite convey what we want here. We don't want people to be in doubt about whether it's safe from the interpreter's point of view. Maybe something along the lines of "it is guaranteed that the current code owns the only reference to the object"?
It's probably also worth mentioning why a refcount of 1 isn't enough to guarantee it's a unique temporary, something about the interpreter borrowing references internally, similar to the whatsnew entry.
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com> Co-authored-by: T. Wouters <thomas@python.org> Co-authored-by: mpage <mpage@cs.stanford.edu>
There was a problem hiding this comment.
I'd prefer PyUnstable_Object_IsUniqueReferencedTemporary as it is not the object that is unique, but the reference to it.
The code seems correct, but could be more efficient. If the total explicit refcount is 1, then the first non-borrowed reference to the object must be the only reference.
| int | ||
| PyUnstable_Object_IsUniqueTemporary(PyObject *op) | ||
| { | ||
| if (!_PyObject_IsUniquelyReferenced(op)) { |
There was a problem hiding this comment.
I think this just demonstrates that _PyObject_IsUniquelyReferenced is badly named, otherwise we wouldn't need a new function.
We need to check that the following are true:
- The object has an explicit (not borrowed) reference count of 1
- It is mortal
- It does not support deferred reclamation.
I think _PyObject_IsUniquelyReferenced does that, but it isn't clear from the name.
There was a problem hiding this comment.
Let's discuss the name for _PyObject_IsUniquelyReferenced in #133144
|
When you're done making the requested changes, leave the comment: |
Co-authored-by: Mark Shannon <mark@hotpy.org>
PyUnstable_Object_IsUniqueTemporary C APIPyUnstable_Object_IsUniqueReferencedTemporary C API
There was a problem hiding this comment.
As expected, the NumPy tests pass if I replace the python internals usage with this function:
diff --git a/numpy/_core/src/multiarray/temp_elide.c b/numpy/_core/src/multiarray/temp_elide.c
index 667ba2b443..ee1194b3a0 100644
--- a/numpy/_core/src/multiarray/temp_elide.c
+++ b/numpy/_core/src/multiarray/temp_elide.c
@@ -62,13 +62,6 @@
#include <feature_detection_misc.h>
-#if PY_VERSION_HEX >= 0x030E00A7 && !defined(PYPY_VERSION)
-#define Py_BUILD_CORE
-#include "internal/pycore_frame.h"
-#include "internal/pycore_interpframe.h"
-#undef Py_BUILD_CORE
-#endif
-
/* 1 prints elided operations, 2 prints stacktraces */
#define NPY_ELIDE_DEBUG 0
#define NPY_MAX_STACKSIZE 10
@@ -124,28 +117,7 @@ check_unique_temporary(PyObject *lhs)
// that an object is a unique temporary variable. We scan the top of the
// interpreter stack to check if the object exists as an "owned" reference
// in the temporary stack.
- PyFrameObject *frame = PyEval_GetFrame();
- if (frame == NULL) {
- return 0;
- }
- _PyInterpreterFrame *f = frame->f_frame;
- _PyStackRef *base = _PyFrame_Stackbase(f);
- _PyStackRef *stackpointer = f->stackpointer;
- int found_once = 0;
- while (stackpointer > base) {
- stackpointer--;
- PyObject *obj = PyStackRef_AsPyObjectBorrow(*stackpointer);
- if (obj == lhs) {
- if (!found_once && PyStackRef_IsHeapSafe(*stackpointer)) {
- found_once = 1;
- }
- else {
- return 0;
- }
- }
- return found_once;
- }
- return 0;
+ return PyUnstable_Object_IsUniqueReferencedTemporary(lhs);
#else
return 1;
#endif
Doc/whatsnew/3.14.rst
Outdated
| compared to previous Python versions. C API extensions that checked | ||
| :c:func:`Py_REFCNT` of ``1`` to determine if an function argument is not | ||
| referenced by any other code should instead use | ||
| :c:func:`PyUnstable_Object_IsUniqueTemporary` as a safer replacement. |
There was a problem hiding this comment.
The fallout of this is a bit wider than the need for this function, e.g. the Pandas check that broke was looking at if sys.refcount(obj) <= 3. There were also some NumPy and nanobind tests that failed because the absolute values of reference counts changed.
https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L4156-L4161
(c.f. pandas-dev/pandas#61368).
I'm actually not totally sure why Pandas was checking for <= 3 in particular and if there's a way for them to implement that check in pure Python still.
In NumPy we switched all the refcount tests to check changes in refcounts before and after an operation rather than absolute refcount values. That works across Python versions.
Maybe a separate paragraph advising against using the absolute values of reference counts in tests or runtime sanity checks? Could be a followup too.
| @@ -89,6 +89,10 @@ If you encounter :exc:`NameError`\s or pickling errors coming out of | |||
| :mod:`multiprocessing` or :mod:`concurrent.futures`, see the | |||
| :ref:`forkserver restrictions <multiprocessing-programming-forkserver>`. | |||
|
|
|||
| The interpreter avoids some reference count modifications internally when | |||
|
@markshannon - would you please take another look at this? |
Co-authored-by: Victor Stinner <vstinner@python.org>
After gh-130704, the interpreter replaces some uses of
LOAD_FASTwithLOAD_FAST_BORROWwhich avoid incref/decrefs by "borrowing" references on the interpreter stack when the bytecode compiler can determine that its safe.This change broke some checks in C API extensions that relied on
Py_REFCNT()of1to determine if it's safe to modify an object in-place. Objects may have a reference count of one, but still be referenced further up the interpreter stack due to borrowing of references.This provides a replacement function for those checks.
PyUnstable_Object_IsUniqueReferencedTemporaryis more conservative: it checks that the object has a reference count of one and that it exists as a unique strong reference in the interpreter's stack of temporary variables in the top most frame.See also:
📚 Documentation preview 📚: