Skip to content
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

feat: Do not use C speedups on Python 3.13+ freethreading #331

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mgorny
Copy link

@mgorny mgorny commented Oct 19, 2024

Do not enable C speedups by default if running Python 3.13+ in freethreading mode (with GIL disabled). Since the extension does not support this mode explicitly at the moment, loading it would cause CPython to reenable GIL with a warning and penalize the whole environment.

Discussed in issue #330.


I've signed the CLA today, but I suppose it will take some time for it to be processed.

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

Please add an entry in CHANGES.rst describing your change. It should tell that for the time being (at least until the C optimization supports freethreading) the use of the C optimization is disabled automatically in freethreading mode.

Do not enable C speedups by default if running Python 3.13+
in freethreading mode (with GIL disabled).  Since the extension
does not support this mode explicitly at the moment, loading it would
cause CPython to reenable GIL with a warning and penalize the whole
environment.

Discussed in issue zopefoundation#330.
@icemac
Copy link
Member

icemac commented Oct 22, 2024

@tseaver @davisagli I thought that the refactoring of the C code was done to support free threading. Am I wrong here?

@davisagli
Copy link
Member

@icemac That's my understanding too. I'll need to find some time to look more closely and see if we just need to declare now that we support it, or if more changes are also needed.

@tseaver
Copy link
Member

tseaver commented Oct 22, 2024

@davisagli

I'll need to find some time to look more closely and see if we just need to declare now that we support it, or if more changes are also needed.
(notes move to #332).

@mgorny
Copy link
Author

mgorny commented Oct 23, 2024

My CLA has just been approved. Should this be merged then, or should I close it pending proper FT support?

@d-maurer
Copy link
Contributor

d-maurer commented Oct 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants