-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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-117142: Port _ctypes to multi-phase init #117181
Conversation
Changing this to a draft. See a separated PR: #117189. |
Modules/_ctypes/_ctypes.c
Outdated
@@ -1150,6 +1147,12 @@ size property/method, and the sequence protocol. | |||
|
|||
*/ | |||
|
|||
/*[clinic input] | |||
class _ctypes.PyCPointerType "PyObject *" "st->PyCPointerType_Type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've used the practice of defining a "clinic state" macro for getting the module state in generated clinic code. It has proven useful if you want to tweak stuff afterwards (smaller diffs, less churn). You can git grep clinic_state **/*.c
for inspiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, *_impl()
should be passed a module state if the AC gets it in advance with some overhead. Also, _ctypes
would need a module state getter to be specified in each function clinic input, making a class input have a type name without any operator, EDIT: or making the getter return st
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, _PyType_GetModuleState(cls)
or _PyType_GetModuleState(cls->tp_base)
can be applied to each class. I'll try.
Is it acceptable to optimize |
I seem to recall it had such an optimization at some point. I'm not sure why it's not there now; perhaps we simply went with simpler code in the first PR. If you can measure the speedup, definitely add it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing, thank you!
I found one more snag, other than that it looks like we're almost home.
And back to optimizing So it seems the current code isn't quite carefully tuned, feel free to make it faster. (But note that getting a type's MRO and getting an item from a tuple should be very fast already.) |
Co-authored-by: Petr Viktorin <[email protected]>
Regarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Thank you very much for the review. |
Good job! |
Port
_ctypes
to multi-phase init with the module state enabled.Module state access:
PyType_GetModuleByDef(Py_TYPE(type))
PyType_GetModuleByDef(Py_TYPE(Py_TYPE(obj)))
_PyType_GetModuleState(Py_TYPE(obj))
_PyType_GetModuleState(Py_TYPE(obj))