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

Allow GIL to be disabled in whirlpool C extension #1344

Merged

Conversation

zmedico
Copy link
Member

@zmedico zmedico commented Jun 13, 2024

In 3.13 python extensions need to declare support for GIL features, for example if they don't declare Py_MOD_GIL_NOT_USED then it will cause the GIL to be enabled even when python was launched in free-threaded mode.

This requires "multi-phase initialization" because Py_mod_create is incompatible with m_slots. There's a PyUnstable_Module_SetGIL() function that can be used with single-phase init, but it's an unstable API, so it's better to use multi-phase init. There's no need to use PyModule_GetState() because the whirlpool module has no mutable state.

Bug: https://bugs.gentoo.org/934220

@zmedico zmedico requested a review from mgorny June 13, 2024 21:13
@zmedico zmedico marked this pull request as draft June 13, 2024 21:18
@thesamesam thesamesam requested a review from floppym June 13, 2024 22:12
@thesamesam
Copy link
Member

Thanks for looking at this already. I haven't dared try -gil yet...

@zmedico zmedico force-pushed the bug_934220_whirlpool_all_gil_disabled branch 2 times, most recently from e42efd5 to d8d1847 Compare June 13, 2024 22:39
@zmedico zmedico marked this pull request as ready for review June 13, 2024 22:40
@zmedico zmedico force-pushed the bug_934220_whirlpool_all_gil_disabled branch from d8d1847 to 78a96aa Compare June 13, 2024 22:41
@zmedico zmedico marked this pull request as draft June 13, 2024 22:51
@zmedico zmedico marked this pull request as ready for review June 13, 2024 23:30
@zmedico zmedico force-pushed the bug_934220_whirlpool_all_gil_disabled branch from 78a96aa to 8eb20d5 Compare June 13, 2024 23:36
@floppym
Copy link
Contributor

floppym commented Jun 14, 2024

Having just given myself a crash course on "multi-phase initialization", this looks ok to me.

@zmedico
Copy link
Member Author

zmedico commented Jun 16, 2024

Having just given myself a crash course on "multi-phase initialization", this looks ok to me.

I've found that there's a PyUnstable_Module_SetGIL() function we can use with single-phase init modules, but it's an unstable API so it seems better to migrate to multi-phase init:

https://docs.python.org/3.13/c-api/module.html#c.PyUnstable_Module_SetGIL

@zmedico zmedico force-pushed the bug_934220_whirlpool_all_gil_disabled branch from 8eb20d5 to 88646d7 Compare June 16, 2024 19:47
@zmedico zmedico requested a review from thesamesam June 16, 2024 19:50
@zmedico
Copy link
Member Author

zmedico commented Jun 16, 2024

Thanks for looking at this already. I haven't dared try -gil yet...

I'm running emerge with PYTHON_GIL=0 EPYTHON=python3.13 now, and I haven't noticed any issues. I suspect all of our code is safe, so I don't expect to see an issue unless it is in python itself.

In 3.13 python extensions need to declare support for GIL features, for
example if they don't declare Py_MOD_GIL_NOT_USED then it will cause the
GIL to be enabled even when python was launched in free-threaded mode.

This requires "multi-phase initialization" because Py_mod_create is
incompatible with m_slots. There's a PyUnstable_Module_SetGIL() function
that can be used with single-phase init, but it's an unstable API, so
it's better to use multi-phase init. There's no need to use
PyModule_GetState() because the whirlpool module has no mutable state.

Bug: https://bugs.gentoo.org/934220
Signed-off-by: Zac Medico <zmedico@gentoo.org>
@zmedico zmedico force-pushed the bug_934220_whirlpool_all_gil_disabled branch from 88646d7 to 918f4a4 Compare June 28, 2024 18:15
@gentoo-bot gentoo-bot merged commit 918f4a4 into gentoo:master Jun 28, 2024
@zmedico zmedico deleted the bug_934220_whirlpool_all_gil_disabled branch June 28, 2024 18:17
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.

4 participants