associate lifetime of PyMethodDef[] and PyTypeObject
I am trying to use the PEP384 style type-construction with PyType_FromSpec. I couldn't find much documentation on this, so first: is this the expected way to make types going forward? I have always used static objects, but this API is actually very convenient for programatically generated types.
As for my actual question: while nested structs like tp_as_number and tp_as_mapping have been broken up such that you just pass Py_nb_add etc., tp_methods still just wants a null-terminated array of PyMethodDef objects. This would be fine, but the resulting PyCFunction objects hold pointers into this array, meaning I must ensure that the array outlives the type. I could put the array in a capsule and assign it to the type, but then if a user deletes the attribute from the type, it will free the memory calling those methods will be a use after free. I could make a metaclass that has room for the array on the type object itself, but it appears that the type object is always allocated with PyType_GenericAlloc(&PyType_type, nmembers) where nmembers is the size of the Py_tp_members slot. Also, even if I could get the space, this would leak an implementation detail into the class hierarchy, which is probably fine but seems messy.
One idea I have is to change PyCFunctionObject to hold a PyMethodDef *value* instead of a pointer. I don't think the extra foot print size will affect much, and will lower the total memory usage of a PyCFunctionObject by one pointer. If anything, it avoids a second indirection to find the function's implementation. I wanted to poll this list before working on this to see if this has already been discussed or if I am just doing something wrong. PyCFunctionObject is not part of the limited ABI, so this should be a stable ABI compatible change. We can make this same change in PyType_Ready for statically allocated types.
On 2019-06-12 03:42, Joe Jevnik via capi-sig wrote:
One idea I have is to change PyCFunctionObject to hold a PyMethodDef *value* instead of a pointer.
We might as well go further and rethink how we store the data provided by PyMethodDef. For example, we could store the function name as Python str object (instead of a C string). I did that in my PEP 580 implementation (which was rejected) and it simplified some code.
On 2019-06-12 15:25, Joe Jevnik wrote:
The goal is to keep things pretty much backwards compatible.
I don't think that my proposal is any more backwards incompatible than yours. So I don't see the problem here.
The PyMethodDef is still part of the public API
Sure. I didn't mean to change that. My suggestion was not to store an exact copy of the PyMethodDef structure inside PyCFunctionObject but instead store a similar-but-not-quite-the-same structure where the name is replaced by a Python object.
On 11Jun2019 1842, Joe Jevnik via capi-sig wrote:
As Jeroen suggested, I think it's not just okay but a *good* thing for PyCFunctionObject to not have to refer back to the PyMethodDef. I've certainly run into this problem before.
We can still use PyMethodDef as the input value, but unless we have a particular public API for retrieving it (which I don't recall we do), it may as well just go away at that point.
My guess is it made perfect sense when everything was static, but no longer does now.
Cheers, Steve
I created an issue and added a PR that I believe covers all of the unmanaged memory. The only thing I know is still unmanaged is the PyGetSetDef closure, but I am not sure what to do about that. Also, users can opt-out of that API so it's less of an issue.
https://bugs.python.org/issue37270
On Wed, Jun 12, 2019 at 2:54 PM Andrew Svetlov <andrew.svetlov@gmail.com> wrote:
On Thu, Jun 13, 2019 at 2:35 PM Joe Jevnik via capi-sig <capi-sig@python.org> wrote:
We encountered a similar issue when switching PyStructureSequence_NewType to use PyType_FromSpec. My colleague Eddie Elizondo put together the pull request that made that change and you can see where we were affected by the same issue with statically allocated data in PyType_Spec and PyMemberDef
https://github.com/python/cpython/pull/9665/files
Our internal version uses the same trick you propose to assign PyUnicode_AsUTF8 to tp_name and it works well. That should find its way upstream.
On Thu, 13 Jun 2019 at 01:59, Steve Dower <steve.dower@python.org> wrote:
+1 - I think these cases can be treated as bugs where the introduction of the stable ABI invalidated old assumptions, but we haven't flushed out and fixed all those outdated assumptions yet.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Hi Joe,
This issue has been fixed before for other slots such as Py_tp_members.
The right solution is to copy the contents of the PyMethodDef array into the heap type object itself. This guarantees that you will have valid pointers throughout the type’s life-cycle and they will be cleaned up once the type’s tp_dealloc function is called. Using this you can deallocate your PyMethodDef array immediately after creating the type.
You’ll need to create a PR to fix the issue by updating PyType_FromSpecWithBases. Feel free to reference this PR where I fixed this very same problem for Py_tp_members: https://github.com/python/cpython/pull/9665/files. Follow the example in PyStructSequence_NewType to dynamically create a PyMemberDef (PyMethodDef in your case), and the change in PyType_FromSpecWithBases that allocates the extra space needed to keep these pointers alive inside the type.
Feel free to ping me if you need any more help.
Best, Eddie
On 2019-06-15 17:25, Eddie Elizondo wrote:
I'm not convinced that this is the "right" solution. This is data which is only used to create the class but it's not used anymore after that. So PyTypeReady() could use tp_methods and create method_descriptor objects but after it's done it could just set tp_methods = NULL.
On 2019-06-12 03:42, Joe Jevnik via capi-sig wrote:
One idea I have is to change PyCFunctionObject to hold a PyMethodDef *value* instead of a pointer.
We might as well go further and rethink how we store the data provided by PyMethodDef. For example, we could store the function name as Python str object (instead of a C string). I did that in my PEP 580 implementation (which was rejected) and it simplified some code.
On 2019-06-12 15:25, Joe Jevnik wrote:
The goal is to keep things pretty much backwards compatible.
I don't think that my proposal is any more backwards incompatible than yours. So I don't see the problem here.
The PyMethodDef is still part of the public API
Sure. I didn't mean to change that. My suggestion was not to store an exact copy of the PyMethodDef structure inside PyCFunctionObject but instead store a similar-but-not-quite-the-same structure where the name is replaced by a Python object.
On 11Jun2019 1842, Joe Jevnik via capi-sig wrote:
As Jeroen suggested, I think it's not just okay but a *good* thing for PyCFunctionObject to not have to refer back to the PyMethodDef. I've certainly run into this problem before.
We can still use PyMethodDef as the input value, but unless we have a particular public API for retrieving it (which I don't recall we do), it may as well just go away at that point.
My guess is it made perfect sense when everything was static, but no longer does now.
Cheers, Steve
I created an issue and added a PR that I believe covers all of the unmanaged memory. The only thing I know is still unmanaged is the PyGetSetDef closure, but I am not sure what to do about that. Also, users can opt-out of that API so it's less of an issue.
https://bugs.python.org/issue37270
On Wed, Jun 12, 2019 at 2:54 PM Andrew Svetlov <andrew.svetlov@gmail.com> wrote:
On Thu, Jun 13, 2019 at 2:35 PM Joe Jevnik via capi-sig <capi-sig@python.org> wrote:
We encountered a similar issue when switching PyStructureSequence_NewType to use PyType_FromSpec. My colleague Eddie Elizondo put together the pull request that made that change and you can see where we were affected by the same issue with statically allocated data in PyType_Spec and PyMemberDef
https://github.com/python/cpython/pull/9665/files
Our internal version uses the same trick you propose to assign PyUnicode_AsUTF8 to tp_name and it works well. That should find its way upstream.
On Thu, 13 Jun 2019 at 01:59, Steve Dower <steve.dower@python.org> wrote:
+1 - I think these cases can be treated as bugs where the introduction of the stable ABI invalidated old assumptions, but we haven't flushed out and fixed all those outdated assumptions yet.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Hi Joe,
This issue has been fixed before for other slots such as Py_tp_members.
The right solution is to copy the contents of the PyMethodDef array into the heap type object itself. This guarantees that you will have valid pointers throughout the type’s life-cycle and they will be cleaned up once the type’s tp_dealloc function is called. Using this you can deallocate your PyMethodDef array immediately after creating the type.
You’ll need to create a PR to fix the issue by updating PyType_FromSpecWithBases. Feel free to reference this PR where I fixed this very same problem for Py_tp_members: https://github.com/python/cpython/pull/9665/files. Follow the example in PyStructSequence_NewType to dynamically create a PyMemberDef (PyMethodDef in your case), and the change in PyType_FromSpecWithBases that allocates the extra space needed to keep these pointers alive inside the type.
Feel free to ping me if you need any more help.
Best, Eddie
On 2019-06-15 17:25, Eddie Elizondo wrote:
I'm not convinced that this is the "right" solution. This is data which is only used to create the class but it's not used anymore after that. So PyTypeReady() could use tp_methods and create method_descriptor objects but after it's done it could just set tp_methods = NULL.
participants (7)
-
Andrew Svetlov -
Carl Shapiro -
Eddie Elizondo -
Jeroen Demeyer -
Joe Jevnik -
Nick Coghlan -
Steve Dower