Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-133164: AddPyUnstable_Object_IsUniqueReferencedTemporary C API#133170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
colesbury merged 13 commits intopython:mainfromcolesbury:gh-133164-unique-temporary
May 2, 2025

Conversation

colesbury
Copy link
Contributor

@colesburycolesbury commentedApr 29, 2025•
edited
Loading

Aftergh-130704, the interpreter replaces some uses ofLOAD_FAST withLOAD_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 onPy_REFCNT() of1 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_IsUniqueReferencedTemporary 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:

📚 Documentation preview 📚:

Afterpythongh-130704, the interpreter replaces some uses of `LOAD_FAST` with`LOAD_FAST_BORROW` which avoid incref/decrefs by "borrowing" referenceson the interpreter stack when the bytecode compiler can determine thatits 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 objectin-place. Objects may have a reference count of one, but still bereferenced further up the interpreter stack due to borrowing ofreferences.This provides a replacement function for those checks.`PyUnstable_Object_IsUniqueTemporary` is more conservative: it checksthat the object has a reference count of one and that it exists as aunique strong reference in the interpreter's stack of temporaryvariables in the top most frame.See also:*numpy/numpy#28681
@colesbury
Copy link
ContributorAuthor

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

colesbury reacted with thumbs up emoji
colesburyand others added2 commitsApril 29, 2025 20:04
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>
Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd preferPyUnstable_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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Let's discuss the name for_PyObject_IsUniquelyReferenced in#133144

ZeroIntensity reacted with thumbs up emoji
@bedevere-app
Copy link

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

Co-authored-by: Mark Shannon <mark@hotpy.org>
@colesburycolesbury changed the titlegh-133164: AddPyUnstable_Object_IsUniqueTemporary C APIgh-133164: AddPyUnstable_Object_IsUniqueReferencedTemporary C APIApr 30, 2025
Copy link
Contributor

@ngoldbaumngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

corona10 and vstinner reacted with thumbs up emoji
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The fallout of this is a bit wider than the need for this function, e.g. the Pandas check that broke was looking at ifsys.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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, I think we should do another pass over this and thePy_REFCNT docs after we addPyUnstable_Object_IsUniquelyReferenced in#133144

ngoldbaum reacted with thumbs up emoji
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👌

@colesbury
Copy link
ContributorAuthor

@markshannon - would you please take another look at this?

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good.

@colesburycolesbury changed the titlegh-133164: AddPyUnstable_Object_IsUniqueReferencedTemporary C APIgh-133164: AddPyUnstable_Object_IsUniqueReferencedTemporary C APIMay 2, 2025
@colesburycolesburyenabled auto-merge (squash)May 2, 2025 13:01
@colesburycolesbury merged commitf237953 intopython:mainMay 2, 2025
42 checks passed
@ngoldbaum
Copy link
Contributor

Awesome! As soon as beta 1 is available for CI I’ll drop support for the 3.14 alpha releases and update NumPy to use this.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@eendebakpteendebakpteendebakpt left review comments

@ngoldbaumngoldbaumngoldbaum left review comments

@Yhg1sYhg1sYhg1s left review comments

@pablogsalpablogsalpablogsal left review comments

@ZeroIntensityZeroIntensityZeroIntensity left review comments

@mpagempagempage approved these changes

@markshannonmarkshannonmarkshannon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

9 participants
@colesbury@ngoldbaum@vstinner@mpage@eendebakpt@Yhg1s@markshannon@pablogsal@ZeroIntensity
[8]ページ先頭
©2009-2025 Movatter.jp