Surface multiphase init #3354
Surface multiphase init #3354
Conversation
…into surface-multiphase-init
oddbookworm
added
Code quality/robustness
| static struct PyModuleDef _module = {PyModuleDef_HEAD_INIT, | ||
| "surface", | ||
| DOC_SURFACE, | ||
| -1, | ||
| 0, |
There was a problem hiding this comment.
why this change? for now I believe it should still be -1 right?
There was a problem hiding this comment.
I’ll double check, but it wouldn’t run for me with -1 because -1 is incompatible with subinterpreters, and multiphase implicitly assumes subinterpreter compat (and I actually forgot to figure out what the actual value should be)
There was a problem hiding this comment.
Yeah, that's what I thought
❯ python -c "import pygame"
Traceback (most recent call last):
File "<string>", line 1, in <module>
import pygame
File "C:\Users\andre\Projects\pygame-ce\src_py\__init__.py", line 166, in <module>
import pygame.display
SystemError: module pygame.surface: m_size may not be negative for multi-phase initialization
|
Hi Andrew. Could you explain to me what this singlephase/multiphase initialization and subinterpreters thing is about? Or if it's easier, where can I properly read about it? I'm uninformed about the topic. |
Here’s the pep for multi phase the reason I want to make this change is because of free threading. Disabling the GIL at a module level in single phase requires the use of unstable API, which can (and probably will) change on us at some arbitrary Python minor version update. Disabling the GIL at a module level in multi phase is as simple as defining a slot |
|
subinterpreters is another topic. The only reason it’s brought up here is because multiphase makes some assumptions that need to be handled for it |
|
Properly handling subinterpreters should be left off for a future pull |
…into surface-multiphase-init
…into surface-multiphase-init
Which part is confusing? |
| c_api[1] = pgSurface_New2; | ||
| c_api[2] = pgSurface_Blit; | ||
| c_api[3] = pgSurface_SetSurface; | ||
| apiobj = encapsulate_api(c_api, "surface"); |
There was a problem hiding this comment.
This is a strong reference presumably, so my hunch is that it should be decref'd because now PyModule_AddObjectRef is not stealing a reference
There was a problem hiding this comment.
Again, as I said before it's probably not a big deal
|
I'm also uncertain about the ref counting in this PR, with PyModule_AddObjectRef. Reading over the multiphase init documentation it mentioned PyModule_AddObjectRef, so maybe Andrew you used it because it was mentioned there? |
Since disabling the GIL on singlephase initialization requires the use of
PyUnstable_*calls, I would like to make use of multiphase initialization whenever possible. Here is a tentative implementation for an initial port forpygame.Surface. Note that this is just for moving over to multiphase initialization and has nothing to do with free-threading. This should not affect current behavior in any way to the best of my understanding