Skip to content
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

Merged
merged 47 commits into from
Apr 10, 2024
Merged

gh-117142: Port _ctypes to multi-phase init #117181

merged 47 commits into from
Apr 10, 2024

Conversation

neonene
Copy link
Contributor

@neonene neonene commented Mar 23, 2024

Port _ctypes to multi-phase init with the module state enabled.

Module state access:

  • Rely on metaclass's mro, which needs to be walked when the C-API metaclass is overridden:
class _swapped_struct_meta(_swapped_meta, type(Structure)):
    pass
class LittleEndianStructure(Structure, metaclass=_swapped_struct_meta):
    ...

# Normal type's mro is less reliable:
type(_SimpleCData)("", (list,), {'_type_': 'i'})
From access
ctype PyType_GetModuleByDef(Py_TYPE(type))
cdata PyType_GetModuleByDef(Py_TYPE(Py_TYPE(obj)))
CFieldObject _PyType_GetModuleState(Py_TYPE(obj)) Internal
CThunkObject _PyType_GetModuleState(Py_TYPE(obj)) Internal

@neonene
Copy link
Contributor Author

neonene commented Mar 24, 2024

Changing this to a draft. See a separated PR: #117189.

@neonene neonene marked this pull request as draft March 24, 2024 07:06
@neonene neonene changed the title gh-117142: Isolate _ctypes gh-117142: Port _ctypes to multi-phase init Mar 29, 2024
@neonene neonene marked this pull request as ready for review March 30, 2024 01:32
@@ -1150,6 +1147,12 @@ size property/method, and the sequence protocol.

*/

/*[clinic input]
class _ctypes.PyCPointerType "PyObject *" "st->PyCPointerType_Type"
Copy link
Contributor

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.

Copy link
Contributor Author

@neonene neonene Apr 5, 2024

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.

Copy link
Contributor Author

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.

@neonene
Copy link
Contributor Author

neonene commented Apr 6, 2024

Is it acceptable to optimize PyType_GetModuleByDef (limited API) by checking the given type before the MRO walk? This PR passes C-API types to it in almost all cases. The tuning in _ctypes (outside PyType_GetModuleByDef) would be fine as well, which should be inlined. I would prefer the former, though.

@encukou
Copy link
Member

encukou commented Apr 8, 2024

Is it acceptable to optimize PyType_GetModuleByDef (limited API) by checking the given type before the MRO walk?

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 :)

Copy link
Member

@encukou encukou left a 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.

Modules/_ctypes/callbacks.c Show resolved Hide resolved
Modules/_ctypes/_ctypes.c Outdated Show resolved Hide resolved
@encukou
Copy link
Member

encukou commented Apr 9, 2024

And back to optimizing PyType_GetModuleByDef,
a similar optimization was added in: #25505
and later removed with no reason in: #30795

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 <encukou@gmail.com>
@neonene
Copy link
Contributor Author

neonene commented Apr 9, 2024

Regarding PyType_GetModuleByDef, I've applied optimization at #117661, which is not beneficial for the getset of ctypes. _decimal got around 7% faster in some cases with noises.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Thank you!

@encukou encukou enabled auto-merge (squash) April 10, 2024 10:54
@encukou encukou merged commit ef41182 into python:main Apr 10, 2024
38 checks passed
@neonene
Copy link
Contributor Author

neonene commented Apr 10, 2024

Thank you very much for the review.

@neonene neonene deleted the ctypes_multi2 branch April 10, 2024 11:22
@erlend-aasland
Copy link
Contributor

Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants