Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 9 commits into from
Apr 4, 2025

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Apr 1, 2025

Micro-optimisation, but we should avoid re-calculating CapsuleType every time it is accessed in the types module.

A

@pitrou
Copy link
Member

pitrou commented Apr 1, 2025

How about exposing CapsuleType directly, for example as sys._CapsuleType, so that we get rid of the _socket dynamic loading hack?

@AA-Turner AA-Turner changed the title Cache types.CapsuleType on first compution gh-109599: Cache types.CapsuleType on first compution Apr 1, 2025
@AA-Turner
Copy link
Member Author

It feels wrong to expose CapsuleType in sys (I don't believe there are any other types defined on the module?). Perhaps the ideal would be a _types extension module that types could import from?

Obviously that would be a much larger change: I can justify backporting this PR as a bugfix, but not a new module!

A

@pitrou
Copy link
Member

pitrou commented Apr 1, 2025

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 sys was just pragmatic, precisely so that we don't create a new private extension module for a single symbol.

@AA-Turner AA-Turner removed the needs backport to 3.13 bugs and security fixes label Apr 1, 2025
@AA-Turner
Copy link
Member Author

True! (removed labels)

Let's see if anyone else has an opinion, otherwise I'll have a look into the sys approach.

A

@picnixz
Copy link
Member

picnixz commented Apr 1, 2025

I don't think it's the correct place but at the same time the sysmodule says:

Various bits of information used by the interpreter are collected in module 'sys'.

So it could be the correct place, albeit not the most intuitive one. Note that we already do SimpleNamespace = type(sys.implementation). However, be careful that the sys variable is explicitly deleted so you might want to need a re-import in __getattr__ anyway as it's lazy (the reason is that it's to avoid exposing the variable as types._sys).

OTOH, we have a TODO note:

class _GeneratorWrapper:
    # TODO: Implement this in C.

so this might also be an opportunity to have a true _types built-in module.

@ZeroIntensity
Copy link
Member

so this might also be an opportunity to have a true _types built-in module.

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.

@AA-Turner AA-Turner requested review from a team, erlend-aasland and corona10 as code owners April 1, 2025 13:44
@AA-Turner
Copy link
Member Author

What do you think of this approach? The compexity would come from converting _GeneratorWrapper. The commits I've just pushed simply expose SimpleNamespace and CapsuleType, and nothing further (the full conversion could be explored in a dedicated PR).

A

Modules/_typesmodule.c Show resolved Hide resolved
@pitrou
Copy link
Member

pitrou commented Apr 1, 2025

This approach looks good to me.

@AA-Turner AA-Turner changed the title gh-109599: Cache types.CapsuleType on first compution gh-109599: Expose CapsuleType via the _types module Apr 1, 2025
@AA-Turner AA-Turner requested a review from picnixz April 1, 2025 14:11
Copy link
Member

@zooba zooba left a 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.

Lib/types.py Outdated Show resolved Hide resolved
`from types import *` will ignore underscore-prefixed names by default, so defining `__all__` doesn't add anything.
@AA-Turner
Copy link
Member Author

AA-Turner commented Apr 1, 2025

From a build perspective I believe it should be configured correctly for static linking, I borrowed the configuration for _typing in large part. A good sign is that '_types' in sys.builtin_module_names is True.

@pitrou
Copy link
Member

pitrou commented Apr 1, 2025

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.

cc @gpshead for a quick look perhaps.

Lib/types.py Show resolved Hide resolved
@ZeroIntensity
Copy link
Member

@AA-Turner Do you mind if I/someone run the buildbots to make sure that the new module builds properly?

@AA-Turner AA-Turner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 3, 2025
@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 3, 2025
@AA-Turner
Copy link
Member Author

AA-Turner commented Apr 3, 2025

The ARM64 macOS failure is in test_idle, and the two ARM64 Windows failures also look unrelated (I think we have known problems on Windows/ARM at present?).

A

Copy link
Member

@ZeroIntensity ZeroIntensity left a 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.

@AA-Turner
Copy link
Member Author

Especially for CapsuleType it's not possible to have a pure-Python fallback. If other implementations don't want to alter types.py then they could e.g. provide the various names in a _types.py which would be picked up, but they will already have strategies for dealing with the general problem.

A

Copy link
Member

@ZeroIntensity ZeroIntensity left a 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!

@AA-Turner AA-Turner merged commit 231a50f into python:main Apr 4, 2025
125 of 128 checks passed
@AA-Turner AA-Turner deleted the opt-types-capsule branch April 4, 2025 22:37
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.