Skip to content

Conversation

@neonene
Copy link
Contributor

@neoneneneonene 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)): passclassLittleEndianStructure(Structure, metaclass=_swapped_struct_meta): ... # Normal type's mro is less reliable:type(_SimpleCData)("", (list,),{'_type_': 'i'})
Fromaccess
ctypePyType_GetModuleByDef(Py_TYPE(type))
cdataPyType_GetModuleByDef(Py_TYPE(Py_TYPE(obj)))
CFieldObject_PyType_GetModuleState(Py_TYPE(obj))Internal
CThunkObject_PyType_GetModuleState(Py_TYPE(obj))Internal

@neonene
Copy link
ContributorAuthor

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

@neoneneneonene marked this pull request as draft March 24, 2024 07:06
@neoneneneonene changed the title gh-117142: Isolate _ctypesgh-117142: Port _ctypes to multi-phase initMar 29, 2024
@neoneneneonene marked this pull request as ready for review March 30, 2024 01:32
*/

/*[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
ContributorAuthor

@neoneneneoneneApr 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
ContributorAuthor

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
ContributorAuthor

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

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

@encukouencukou 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.

@encukou
Copy link
Member

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
ContributorAuthor

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

@encukouencukou 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!

@encukouencukou enabled auto-merge (squash) April 10, 2024 10:54
@encukouencukou merged commit ef41182 into python:mainApr 10, 2024
@neonene
Copy link
ContributorAuthor

Thank you very much for the review.

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

Good job!

Sign up for freeto 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

@neonene@encukou@erlend-aasland