This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: [C API] Add new C functions with more regular reference counting like PyTuple_GetItemRef()
Type: Stage: resolved
Components: C API Versions: Python 3.10
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, kj, ronaldoussoren, serhiy.storchaka, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-11-09 09:42 by vstinner, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 23206 merged vstinner, 2020-11-09 11:50
PR 23207 closed vstinner, 2020-11-09 13:09
PR 23209 closed vstinner, 2020-11-09 14:19
PR 23227 merged kj, 2020-11-10 23:56
Messages (12)
msg380578 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-09 09:42
The C API of Python uses and abuses borrowed references and stealing references for performance. When such function is used in some very specific code for best performances, problems arise when they are the only way to access objects. Reference counting in C is error prone, most people, even experimented core developers, get it wrong. Examples of issues:

* Reference leaks: objects are never deleted causing memory leaks. For example, an error handling code which forgets to call Py_DECREF() on a newly create object.

* Unsafe borrowed references: call arbitrary Python code can delete the referenced objects, and so the borrowed reference becomes a dangling pointer. Most developers are confident that a function call cannot run arbitrary Python code, whereas a single Py_DECREF() can trigger a GC collection which runs finalizers which can be arbitrary Python code. Many functions have been fixed manually by adding Py_INCREF() and Py_DECREF() around "unsafe" function calls.


Borrowed references and stealing references make reference counting code special, even more complex to review. I propose to use new function to make refecence counting code more regular, simpler to review, and so less error prone.

Examples:

* Add PyTuple_GetItem(): similar to PyTuple_GetItem() but returns a strong reference (or NULL if the tuple item is not set)
* Add PyTuple_SetItemRef(): similar to PyTuple_SetItem() but don't steal a reference to the new item

The C API has a long list of functions using borrowed references, so I'm not sure where we should stop. I propose to start with the most common functions: PyDict, PyTuple, PyList, and see how it goes.

--

PyTuple_GetItem() is a function call which checks arguments: raise an exception if arguments are invalid. For best performances, PyTuple_GET_ITEM() macro is providing to skip these checks. This macro also returns a borrowed reference.

I'm not if a new PyTuple_GET_ITEM_REF() macro should be added: similar to PyTuple_GET_ITEM() but returns a strong reference.

Same open question abut PyTuple_SET_ITEM(tuple, index, item) macro which is also special:

* Don't call Py_XINCREF(item)
* Don't call Py_XDECREF() on the old item

If a new PyTuple_SET_ITEM_REF() macro is added, I would prefer to make the function more "regular" in term of reference counting, and so call Py_XDECREF() on the old item. When used on a newly created tuple, it would add an useless Py_XDECREF(NULL), compared to PyTuple_SET_ITEM(). Again, my idea here is to provide functions with a less surprising behavior and more regular reference counting. There are alternatives to build a new tuple without the useless Py_XDECREF(NULL), like Py_BuildValue().

Code which requires best performance could continue to use PyTuple_SET_ITEM() which is not deprecated, and handle reference counting manually.

--

An alternative is to use abstract functions like:

* PyTuple_GetItem() => PySequence_GetItem()
* PyDict_GetItem() => PyObject_GetItem()
* etc.

I propose to keep specialized functions per type to avoid the overhead of indirection. For example, PySequence_GetItem(obj, index) calls Py_TYPE(obj)->tp_as_sequence->sq_item(obj, index) which implies multiple indirection:

* Get the object type from PyObject.ob_type
* Dereference *type to get PyTypeObject.tp_as_sequence
* Dereference *PyTypeObject.tp_as_sequence to get PySequenceMethods.sq_item

--

I don't plan to get rid of borrowed references. Sometimes, they are safe and replacing them with strong references would require explicit reference counting code which is again easy to get wrong.

For example, Py_TYPE() returns a borrowed reference to an object type. The function is commonly used to access immediately to a type member, with no risk of calling arbitrary Python code between the Py_TYPE() call and the read of the type attribute. For example, the following code is perfectly safe:

        PyErr_Format(PyExc_TypeError, "exec() globals must be a dict, not %.100s",
                     Py_TYPE(globals)->tp_name);


--

See also bpo-42262 where I added Py_NewRef() and Py_XNewRef() functions.

See https://pythoncapi.readthedocs.io/bad_api.html#borrowed-references for details about issues caused by borrowed references and a list of functions using borrowed references.
msg380583 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-09 12:40
New changeset 23c5f93b83f78f295313e137011edb18b24c37c2 by Victor Stinner in branch 'master':
bpo-42294: Add borrowed/strong reference to doc glossary (GH-23206)
https://github.com/python/cpython/commit/23c5f93b83f78f295313e137011edb18b24c37c2
msg380592 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-11-09 14:56
I'm not convinced that this is worth the effort.

The old functions aren't going away, so these additional functions provide no real safety.
You can't stop C programmers trading away correctness for some perceived performance benefit :(

If we were designing the API from scratch, then this would be a better set of functions. But because the old functions remain, it just means we are making the API larger.


Please don't add macros, use inline functions.

There seems to be some confusion about borrowed references and stolen references in https://pythoncapi.readthedocs.io/bad_api.html#borrowed-references
"Stealing" a reference is perfectly safe. Returning a "borrowed" reference is not.

So, don't bother with `PyTuple_SetItemRef()`, as `PyTupleSetItem()` is safe.
msg380593 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-09 15:20
Mark Shannon:
> The old functions aren't going away, so these additional functions provide no real safety. You can't stop C programmers trading away correctness for some perceived performance benefit :(

In my experience, newcomers tend to copy existing more. If slowly, the code base moves towards safer code less error-prone code like Py_NewRef() or Py_SETREF(), slowly, we will avoid a bunch of bugs.


> If we were designing the API from scratch, then this would be a better set of functions. But because the old functions remain, it just means we are making the API larger.

New API VS enhance the existing API. So far, no approach won. I wrote the PEP 620 to enhance the C API and towards a more opaque API, and there is the HPy project which is a new API written correctly from the start. But HPy is not usable yet, and migrating C extensions to HPy will take years.

Also, enhancing the existing API and writing a new API are not exclusive option.

What is the issue of making the C API larger?


> Please don't add macros, use inline functions.

For Py_NewRef(), I used all at once :-) static inline function + function + macro :-)

It's exported as a regular function for the stable ABI, but overriden by a static inline function with a macro. The idea is to allow to use it for developers who cannot use static inline functions (ex: extension modules not written in C).

I chose to redefine functions as static inline functions in the limited C API. If it's an issue, we can consider to only do that in Include/cpython/object.h.


> There seems to be some confusion about borrowed references and stolen references in https://pythoncapi.readthedocs.io/bad_api.html#borrowed-references
"Stealing" a reference is perfectly safe. Returning a "borrowed" reference is not.
>
> So, don't bother with `PyTuple_SetItemRef()`, as `PyTupleSetItem()` is safe.

I'm really annoyed that almost all functions increase the refcount of their arugments, except a bunch of special cases. I would like to move towards a more regular API.

PyTuple_SetItem() is annoying because it steals a reference to the item. Also, it doesn't clear the reference of the previous item, which is also likely to introduce a reference leak.
msg380594 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-11-09 15:31
PyTuple_SetItem() does clear the previous item, it uses Py_XSETREF. The macro version (PyTuple_SET_ITEM) does not clear the previous item.
msg380599 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-09 17:31
> PyTuple_SetItem() does clear the previous item, it uses Py_XSETREF. The macro version (PyTuple_SET_ITEM) does not clear the previous item.

Oh sorry, I was thinking at PyTuple_SET_ITEM().
msg380621 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-11-09 22:22
I concur with Mark.

If you want to work only with non-borrowed references, use PySequence_GetItem() and PySequence_SetItem(). It has a cost: it is slower and needs checking errors. If you need more performant solution and binary compatibility across versions, use PyTuple_GetItem() and PyTuple_SetItem() (borrowed references is the part of optimization). If you don't need binary compatibility, but need speed, use macros.

And no need to expand the C API. It is already large enough.
msg380729 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-10 23:57
New changeset 78ba7c69ada2f7eefd4e3ea375337d78dc2ce721 by kj in branch 'master':
bpo-42294: Grammar fixes in doc glossary strong/weak refs (GH-23227)
https://github.com/python/cpython/commit/78ba7c69ada2f7eefd4e3ea375337d78dc2ce721
msg380730 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-11 00:34
In bpo-1635741, I added PyModule_AddObjectRef() (commit 8021875bbcf7385e651def51bc597472a569042c):
https://docs.python.org/dev/c-api/module.html#c.PyModule_AddObjectRef

"Similar to PyModule_AddObject() but don't steal a reference to the value on success."

I was tired of bugs caused by misusage of the surprising PyModule_AddObject() API.

PyModule_AddObject() *is* useful in some cases, but it is confusing in most cases...
msg380731 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-11 00:36
> If you want to work only with non-borrowed references, (...)

This is not my goal here. My goal is to reduce the risk of memory leaks.
msg380735 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-11-11 06:47
PyModule_AddObject() has unique weird design, it is easy to misuse, and most code misuse it, but fixing it would break the code which uses it correctly.

I did not see any problems with PyTuple_GetItem().
msg402372 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-09-21 21:51
There is no consensus on changing things, so I just close my issue.
History
Date User Action Args
2022-04-11 14:59:37adminsetgithub: 86460
2021-09-21 21:51:54vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg402372

stage: patch review -> resolved
2020-11-11 06:47:15serhiy.storchakasetmessages: + msg380735
2020-11-11 00:36:09vstinnersetmessages: + msg380731
2020-11-11 00:34:03vstinnersetmessages: + msg380730
2020-11-10 23:57:03vstinnersetmessages: + msg380729
2020-11-10 23:56:22kjsetnosy: + kj
pull_requests: + pull_request22130
2020-11-09 22:22:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg380621
2020-11-09 17:31:27vstinnersetmessages: + msg380599
2020-11-09 16:55:18shihai1991setnosy: + shihai1991
2020-11-09 15:31:06ronaldoussorensetnosy: + ronaldoussoren
messages: + msg380594
2020-11-09 15:20:48vstinnersetmessages: + msg380593
2020-11-09 14:56:57Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg380592
2020-11-09 14:19:38vstinnersetpull_requests: + pull_request22107
2020-11-09 13:09:25vstinnersetpull_requests: + pull_request22105
2020-11-09 12:40:56vstinnersetmessages: + msg380583
2020-11-09 11:50:47vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request22104
2020-11-09 09:42:14vstinnercreate