bpo-38206: Clarify tp_dealloc requirements for heap allocated types. #16248
Conversation
|
Happy to take any suggestions on the wording. |
|
Yup! Looks mostly good, just a couple of comments. |
|
One last comment. This looks good now! Ping @DinoV , could you help getting this merged? |
| static void foo_dealloc(foo_object *self) { | ||
| PyTypeObject *tp = Py_TYPE(self); | ||
| // free references and buffers here | ||
| tp->tp_free(self); |
There was a problem hiding this comment.
Since this is a heap type, change this to use the slot API to be compliant with PEP384.
freefunc free_func = PyType_GetSlot(tp, Py_tp_free);
free_func(self);
There was a problem hiding this comment.
PEP 384 defines a stable ABI – an API subset you can use when you can't recompile extensions for each minor version of CPython.
How is it relevant here?
There was a problem hiding this comment.
@encukou it is very relevant! What you said is correct, but this PEP has the added benefit of making PyTypeObject opaque. This gives the type system control over the implementation of the type object. For instance, in our system, the type object has a very different implementation. Yet, we can correctly interface with C Extensions. Using tp_free directly ties the code to CPython's implementation.
By the way, this is only relevant for C extensions, that is, cpython/Modules + third-party libs. Internal code CPython code should be free to use all the implementation details it wants! Given that the updated doc has the context of heap types, it's a fair assumption to make :-)
There was a problem hiding this comment.
I agree that using the stable ABI is good, and we all migrate to it eventually. However, it's orthogonal to this change, which is about making the documentation correct. Pushing a stable ABI agenda in a fix for "Clarify that tp_dealloc must decref for heap allocated type" seems too sneaky for me – even though I count myself as a proponent of the stable ABI :)
In CPython, PyType_GetSlot is likely* much slower than using the slot directly, so it is not a clear win.
I'm for avoiding direct slot access (both in stdlib and extension modules), but only after a wider discussion and perhaps after also adding a fast (and unstable) API.
And again: the text above talks about PyTypeObject.tp_free. The example is confusingly out of sync with that. I think we should change both occurences, but please, do it in a different PR.
- (I haven't measured, but there's a bunch of extra checks in CPython's
PyType_GetSlot)
There was a problem hiding this comment.
Reverted back to tp->tp_free(self);
There was a problem hiding this comment.
Woops, didn't mean to try any sneaky stuff! 😛
I agree with your point, let's get to anything related to PEP384 independently.
ammaraskar
force-pushed
the
decref_heap
branch
from
73562a2 to
291258b
Compare
|
Thanks @ammaraskar for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
GH-16436 is a backport of this pull request to the 3.8 branch. |
…H-16248) As mentioned in the bpo ticket, this mistake came up on two reviews: - https://github.com/python/cpython/pull/16127GH-pullrequestreview-288312751 - https://github.com/python/cpython/pull/16071GH-pullrequestreview-287819525 Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry. https://bugs.python.org/issue38206 Automerge-Triggered-By: @encukou (cherry picked from commit 5faff97) Co-authored-by: Ammar Askar <[email protected]>
…ythonGH-16248) As mentioned in the bpo ticket, this mistake came up on two reviews: - python#16127 (review) - python#16071 (review) Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry. https://bugs.python.org/issue38206 Automerge-Triggered-By: @encukou
As mentioned in the bpo ticket, this mistake came up on two reviews:
Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry.
https://bugs.python.org/issue38206
Automerge-Triggered-By: @encukou