-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-109599: Expose CapsuleType
via the _types
module
#131969
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
Conversation
How about exposing |
types.CapsuleType
on first computiontypes.CapsuleType
on first compution
It feels wrong to expose Obviously that would be a much larger change: I can justify backporting this PR as a bugfix, but not a new module! A |
A micro-optimization is not a bugfix, so you shouldn't backport this PR in any case :) The proposal to add a private symbol in |
True! (removed labels) Let's see if anyone else has an opinion, otherwise I'll have a look into the A |
I don't think it's the correct place but at the same time the sysmodule says:
So it could be the correct place, albeit not the most intuitive one. Note that we already do OTOH, we have a TODO note: class _GeneratorWrapper:
# TODO: Implement this in C. so this might also be an opportunity to have a true |
Which I implemented a few months ago. I did it for fun to see what kind of speedups it would provide, but didn't think the added complexity was worth upstreaming. |
What do you think of this approach? The compexity would come from converting A |
This approach looks good to me. |
types.CapsuleType
on first computionCapsuleType
via the _types
module
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.
I like this approach, though would want someone else's signoff because I'm not as familiar with the Modules/Setup
changes. Provided the module is always statically linked, it ought to be fine.
`from types import *` will ignore underscore-prefixed names by default, so defining `__all__` doesn't add anything.
From a build perspective I believe it should be configured correctly for static linking, I borrowed the configuration for |
cc @gpshead for a quick look perhaps. |
@AA-Turner Do you mind if I/someone run the buildbots to make sure that the new module builds properly? |
🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit b7f5df1 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131969%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
The ARM64 macOS failure is in A |
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.
Should we add pure-Python fallbacks for when _types
isn't available?
On the one hand, it's good for other implementations that want to provide the types
module, but on the other hand, types
is pretty CPython-specific anyway; it would probably need impl-specific changes, even if it's pure-Python.
LGTM otherwise.
Especially for A |
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.
Sounds good. The implementation looks fine, and unlike most C accelerators, I think _types
can actually simplify some of the code, because we don't have to use silly tricks to get the types at runtime.
Thanks for doing this!
Micro-optimisation, but we should avoid re-calculating
CapsuleType
every time it is accessed in thetypes
module.A