gh-114314: ctypes: remove stgdict and switch to heap types#116458
gh-114314: ctypes: remove stgdict and switch to heap types#116458encukou merged 60 commits intopython:mainfrom
Conversation
In this bootstrap commit, PyStgInfo is part of StgDictObject, but that's a detail hidden by accessor functions. (In the accessors, `state` is currently unused.)
encukou
left a comment
encukou
left a comment
There was a problem hiding this comment.
The old StgDict API was designed to be as fast as possible. Could we not just assert() that the result is >= 0 for these calls?
I'd like to think less about which functions can currently fail and which can't. Where possible, I'd like the to compiler figure that out for me.
Also, I don't want to micro-optimize without a benchmark. With branch prediction (and especially with LTO), I don't think the < 0 check is significant. I didn't see a significant change when running all of test_capi.
I did get a significant speedup by putting these functions in the header, and making them static inline. Let's do that. Incidentally, that'll probably also let the compiler figure out that the -1 doesn't happen.
We've had some segfaults caused by module access in tp_dealloc recently. We should make sure this is really safe.
I made it to leak rather than crash now. Let's leave a proper fix to another PR -- this can't be tested when the state is still global.
|
|
||
| static PyType_Slot ctype_type_slots[] = { | ||
| {Py_tp_traverse, CType_Type_traverse}, | ||
| {Py_tp_clear, CType_Type_clear}, |
There was a problem hiding this comment.
That would indeed be clearer! I'd prefer leaving it to a follow-up PR though: it'll be easier to debug when we get to isolating the module state.
Co-authored-by: Erlend E. Aasland <[email protected]>
erlend-aasland
left a comment
erlend-aasland
left a comment
There was a problem hiding this comment.
I did get a significant speedup by putting these functions in the header, and making them static inline. Let's do that. Incidentally, that'll probably also let the compiler figure out that the -1 doesn't happen.
Yes, putting the PEP-697 wrappers in the header is a neat solution!
I finished an initial read-through of _ctypes.c, and it looks good to me. I'll try to find the time to read through it a couple of times more.
Co-authored-by: neonene <[email protected]>
|
Impressive work; good job! Very inspiring. |
encukou
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Yay! Well done! |
4 participants
Before this change, ctypes classes used a custom dict subclass,
StgDict,as their
tp_dict. This acts like a regular dict but also includes extra informationabout the type.
This replaces stgdict by
StgInfo, a C struct on the type, accessed byPyObject_GetTypeData()(PEP-697).All usage of
StgDict(mainly variables namedstgdict,dict,edictetc.) isconverted to
StgInfo(namedstginfo,info,einfo, etc.).Where the dict is actually used for class attributes (as a regular PyDict), it's now
called
attrdict.This change -- not overriding
tp_dict-- is made to make me comfortable withthe next part of this PR: moving the initialization logic from
tp_newtotp_init.The
StgInfois set up in__init__of each class, with a guard that preventscalling
__init__more than once. Note that abstract classes (likeArrayorStructure) are created usingPyType_FromMetaclassand do not have__init__called.Previously, this was done in
__new__, which also wasn't called for abstractclasses.
Since
__init__can be called from Python code or skipped, there is a testedguard to ensure
StgInfois initialized exactly once before it's used.Thanks to @neonene and @erlend-aasland.