FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-117142: Port _ctypes to multi-phase init by neonene · Pull Request #117181 · python/cpython · GitHub

/ cpython Public

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

Merged
encukou merged 47 commits into
python:mainfrom
neonene:ctypes_multi2
Apr 10, 2024
ConversationCommits (47)ChecksFiles changed
Merged

gh-117142: Port _ctypes to multi-phase init#117181
encukou merged 47 commits into
python:mainfrom
neonene:ctypes_multi2

Conversation

neonene commented Mar 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

bedevere-app Bot mentioned this pull request Mar 23, 2024
Closed
2 tasks
neonene mentioned this pull request Mar 23, 2024
Merged
neonene mentioned this pull request Mar 24, 2024
Merged

neonene commented Mar 24, 2024

Copy link
Copy Markdown
Contributor Author

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

neonene marked this pull request as draft March 24, 2024 07:06
neonene changed the title gh-117142: Isolate _ctypes gh-117142: Port _ctypes to multi-phase init Mar 29, 2024
neonene marked this pull request as ready for review March 30, 2024 01:32
erlend-aasland reviewed Apr 4, 2024
View reviewed changes
Comment thread Modules/_ctypes/_ctypes.c Outdated
*/

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

erlend-aasland Apr 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

neonene Apr 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

neonene Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, _PyType_GetModuleState(cls) or _PyType_GetModuleState(cls->tp_base) can be applied to each class. I'll try.

neonene commented Apr 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Apr 8, 2024

Copy link
Copy Markdown
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 :)

encukou reviewed Apr 8, 2024
View reviewed changes
Comment thread Modules/_ctypes/_ctypes.c
encukou reviewed Apr 9, 2024
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks amazing, thank you!
I found one more snag, other than that it looks like we're almost home.

Comment thread Modules/_ctypes/callbacks.c
Comment thread Modules/_ctypes/_ctypes.c Outdated

encukou commented Apr 9, 2024

Copy link
Copy Markdown
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.)

Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED
f58ed21 
Co-authored-by: Petr Viktorin <encukou@gmail.com>

neonene commented Apr 9, 2024

Copy link
Copy Markdown
Contributor Author

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.

encukou approved these changes Apr 10, 2024
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you!

encukou enabled auto-merge (squash) April 10, 2024 10:54
encukou merged commit ef41182 into python:main Apr 10, 2024

neonene commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

Thank you very much for the review.

neonene deleted the ctypes_multi2 branch April 10, 2024 11:22

erlend-aasland commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

Good job!

neonene mentioned this pull request Apr 17, 2024
Open
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
pythongh-117142: Port _ctypes to multi-phase init (pythonGH-117181)
89c8b7b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

erlend-aasland erlend-aasland left review comments
encukou encukou approved these changes
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL