GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117142: Port _ctypes to multi-phase init by neonene · Pull Request #117181 · python/cpython

Conversation

@neonene

@neonene neonene commented Mar 23, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

  • Issue: Convert _ctypes extension module to multi-phase init #117142

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
neonene added 10 commits March 23, 2024 22:13
@neonene
add test
9f64b8d
@neonene
multi phase init
22d291c
@neonene
_PyType_GetModuleState() for CField,CThunk
c63737e
@neonene
PyType_GetModuleByDef() for cast()
6a775bc
@neonene
get_module_state()
622e09d
@neonene
get_module_state_by_def(Py_TYPE(Py_TYPE(obj)))
5dae271
@neonene
get_module_state_by_def(Py_TYPE(type))
2263f82
@neonene
split GenericPyCData_new()
1877767
@neonene
move global vars to module state
6d97625
@neonene
prepend module state to func args
6a09cad
@bedevere-app bedevere-app Bot added the awaiting review label Mar 23, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Mar 23, 2024
Convert _ctypes extension module to multi-phase init #117142
Closed
2 tasks done
neonene added 2 commits March 24, 2024 00:18
@neonene
fix typos
c24df4d
@neonene
ignored.tsv
678ffa0
@neonene
neonene requested a review from ericsnowcurrently as a code owner March 23, 2024 15:41
@neonene neonene mentioned this pull request Mar 23, 2024
gh-117142: ctypes: Unify meta tp slot functions #117143
Merged
neonene added 2 commits March 24, 2024 03:22
@neonene
_PyType_GetModuleState() with assert
83ec018
@neonene
get_module_state_by_cls -> _by_class
012f8b8
@neonene neonene mentioned this pull request Mar 24, 2024
gh-117142: ctypes: Migrate global vars to module state #117189
Merged
@neonene

neonene commented Mar 24, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene
neonene marked this pull request as draft March 24, 2024 07:06
@bedevere-app bedevere-app Bot removed the awaiting review label Mar 24, 2024
neonene and others added 3 commits March 28, 2024 22:41
@neonene
assert(module) in create_pointer_type()
28696c2
@neonene
sync styles with #117189
a228d35
@neonene
Merge branch 'main' into ctypes_multi2
03c1f79
@neonene neonene changed the title gh-117142: Isolate _ctypes gh-117142: Port _ctypes to multi-phase init Mar 29, 2024
blurb-it Bot and others added 3 commits March 29, 2024 12:21
@blurb-it
📜🤖 Added by blurb_it.
de5110f
@neonene
PyType_GetModuleByDef() for fini
4d69bdc
@neonene
fix warning
f2655eb
@neonene
neonene marked this pull request as ready for review March 30, 2024 01:32
erlend-aasland
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 erlend-aasland Apr 4, 2024

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
neonene reacted with eyes emoji
All reactions
  • 👀 1 reaction

@neonene neonene Apr 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@neonene neonene Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
neonene added 6 commits April 5, 2024 16:38
@neonene
fill type names
5db3c42
@neonene
Merge branch 'main' into ctypes_multi2
5d09e4e
@neonene
Merge branch 'main' into ctypes_multi2
8da8b43
@neonene
AC: class default module state getters
40cec34
@neonene
typo
893a981
@neonene
move define up
807c6c0
@neonene

neonene commented Apr 6, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

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 :)
neonene reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou
encukou reviewed Apr 8, 2024
View reviewed changes
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
encukou
encukou reviewed Apr 9, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_ctypes/callbacks.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@encukou

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.)
neonene reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene @encukou
Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED
f58ed21 
Co-authored-by: Petr Viktorin <[email protected]>
@neonene

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
neonene added 5 commits April 9, 2024 22:11
@neonene
Move PyGILState_Ensure/Release
cb88db3
@neonene
fix a comment
4237557
@neonene
Merge branch 'main' into ctypes_multi2
4802eb0
@neonene
change NEWS (PEP 687 -> 489)
03cffa9
@neonene
typo
76a2b8c
encukou
encukou approved these changes Apr 10, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland and ericsnowcurrently reacted with hooray emoji neonene reacted with heart emoji
All reactions
  • 🎉 2 reactions
  • ❤️ 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Apr 10, 2024
@encukou
encukou enabled auto-merge (squash) April 10, 2024 10:54
@encukou
encukou merged commit ef41182 into python:main Apr 10, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 10, 2024
@neonene

neonene commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

Thank you very much for the review.
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene
neonene deleted the ctypes_multi2 branch April 10, 2024 11:22
@erlend-aasland

erlend-aasland commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

Good job!
neonene reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene neonene mentioned this pull request Apr 17, 2024
[subinterpreters] Refleaks on Windows Under Specific Conditions #117936
Open
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@neonene @diegorusso
pythongh-117142: Port _ctypes to multi-phase init (pythonGH-117181)
89c8b7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@neonene @encukou @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.