Skip to content

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 29, 2022


OverlappedState *st = overlapped_get_state(module);
st->overlapped_type = (PyTypeObject *)PyType_FromModuleAndSpec(
PyTypeObject *overlapped_type = (PyTypeObject *)PyType_FromModuleAndSpec(
Copy link
Member

Choose a reason for hiding this comment

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

Is PyType_FromModuleAndSpec the correct API to retrieve a type object from a module by spec? It calls PyType_FromMetaclass, which does a lot of extra work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the PyType_From* APIs call PyType_FromMetaclass. The recent changes to the PyType_From* APIs should not result in a lot of extra work. If that is the case, PyType_FromMetaclass should be optimised, if possible.

Copy link
Member

Choose a reason for hiding this comment

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

I was under the assumption that the PyType_From* APIs are used to create a new type object from a spec, not fetch an existing type. It may even leak memory . PyType_FromMetaclass calls PyMem_Malloc and eventually assigns it to type->tp_name eventually without a check.

Copy link
Member

Choose a reason for hiding this comment

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

Each call allocates a whole new type on the heap memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, if there are issues with PEP-489 and the PyType_From* APIs they should be addressed in a separate issue and discussed there. Suggesting to mark this conversation as resolved and continue this discussion in an appropriate issue if needed.

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.

@erlend-aasland
Copy link
Contributor Author

Thanks for the review!

@erlend-aasland erlend-aasland merged commit 655b3aa into python:main Jun 29, 2022
@erlend-aasland erlend-aasland deleted the no-overlapped-state-for-you branch June 29, 2022 10:24
@erlend-aasland erlend-aasland linked an issue Jun 29, 2022 that may be closed by this pull request
2 tasks
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Jun 30, 2022
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.

Extension modules with unneeded module state

4 participants