Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-128844: Make _Py_TryIncref public as an unstable API. #128926

Merged
merged 14 commits into from
Jan 28, 2025

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Jan 16, 2025

This exposes _Py_TryIncref as PyUnstable_TryIncref() and the helper function _PyObject_SetMaybeWeakref as PyUnstable_EnableTryIncRef.

These are helpers for dealing with unowned references in a safe way, particularly in the free threading build.


📚 Documentation preview 📚: https://cpython-previews--128926.org.readthedocs.build/en/128926/c-api/object.html#c.PyUnstable_TryIncRef

This exposes `_Py_TryIncref` as `PyUnstable_TryIncref()` and the helper
function `_PyObject_SetMaybeWeakref` as `PyUnstable_EnableTryIncRef`.

These are helpers for dealing with unowned references in a safe way,
particularly in the free threading build.
@corona10 corona10 requested review from vstinner and encukou January 17, 2025 13:55

.. versionadded:: 3.14

.. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe void?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just have a suggestion for the test.

@colesbury colesbury marked this pull request as ready for review January 17, 2025 18:00

PyUnstable_EnableTryIncRef(op);
#ifdef Py_GIL_DISABLED
// PyUnstable_EnableTryIncRef sets the shared flags to *at least*
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems incomplete, can you elaborate it a bit more?

Comment on lines 635 to 636
This is intended as a building block for safely dealing with unowned
references without the overhead of creating a :c:type:`!PyWeakReference`.
Copy link
Contributor

Choose a reason for hiding this comment

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

In C API docs the term borrowed reference is more common than unowned reference, can you use that?

There's a glossary entry for it so it can be linked to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the term, but I don't love the use of "borrowed reference" here. This seems different to me than the typical use of "borrowed reference" -- there's not a particular reference that's borrowed here. It's more like a building block for weak reference like objects without using PyWeakReference.

* Note that this is a stealth reference: wr_object's refcount is

@encukou
Copy link
Member

encukou commented Jan 18, 2025 via email

Comment on lines 635 to 637
This is intended as a building block for safely dealing with
:term:`borrowed references <borrowed reference>` without the overhead of
creating a :c:type:`!PyWeakReference`.
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 should use weak reference, as a generic term. The “without the overhead” note implies that we don't mean the concrete PyWeakReference.
“Borrowed reference” has a specific meaning in CPython (in particular, obj of PyUnstable_TryIncRef is a borrowed reference).
“Unowned reference” would, IMO, need a definition.

I'd also warn that you typically need to control the deallocator (both TryIncRef and tp_dealloc need the same lock). Is that correct?

Suggested change
This is intended as a building block for safely dealing with
:term:`borrowed references <borrowed reference>` without the overhead of
creating a :c:type:`!PyWeakReference`.
This is intended as a building block for managing weak references without the overhead of
creating a :c:type:`!PyWeakReference`.
Typically, correct use of this function requires support from *obj*'s
deallocator (:c:member:`~PyTypeObject.tp_dealloc`).

Here is a larger change, with an example adapted from the issue (itself based on pybind11). I personally didn't “get” the function before seeing the example, but I don't know if I'm the target audience. I'm also not sure if there are other use cases this doesn't cover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I think this is better, especially with the example. I think we should also include the use of PyUnstable_EnableTryIncref in the example.

Copy link
Member

Choose a reason for hiding this comment

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

I've updated the branch.

@colesbury colesbury force-pushed the gh-1288440-try-incref branch from f5507d8 to 405952c Compare January 20, 2025 15:08
@colesbury colesbury requested a review from encukou January 22, 2025 14:51
@colesbury
Copy link
Contributor Author

@encukou - would you please take another look at this when you get a chance?

Copy link
Member

@encukou encukou 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, thank you!
One more nitpick.

Co-authored-by: Petr Viktorin <[email protected]>
@colesbury colesbury force-pushed the gh-1288440-try-incref branch from 78d29a3 to 4a87acb Compare January 28, 2025 14:48
@colesbury colesbury enabled auto-merge (squash) January 28, 2025 19:09
@colesbury colesbury merged commit d23f570 into python:main Jan 28, 2025
41 checks passed
@vstinner
Copy link
Member

Congrats Sam! Honestly, this API is weird/surprising (trying to INCREF, hu?), but it looks useful for Free Threading. The doc is nice, thanks @encukou for the example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants