implement multi-phase init#1024
Conversation
There was a problem hiding this comment.
since we're changing how the subinterpreter support is flagged, i wanted some tests to show it's still working
provinzkraut
force-pushed
the
multi-phase-init
branch
2 times, most recently
from
d65661e to
16fe68d
Compare
| return 0; | ||
| } | ||
|
|
||
| static struct PyModuleDef msgspecmodule = { |
There was a problem hiding this comment.
This was simply moved down to make the flow easier, as it references _core_exec
provinzkraut
force-pushed
the
multi-phase-init
branch
from
74741fb to
06be1a7
Compare
Siyet
left a comment
Siyet
left a comment
There was a problem hiding this comment.
Leaving a note for the history / backlink: this obsoletes the PyState_AddModule workaround jcrist added in #561, where the body said "in the long run we should move to using multiphase module init which should avoid this problem entirely." Good to have the thread connected.
please do ping if/when that happens 🙏🏼 |
|
@provinzkraut Happy to push these small fixups to your branch myself if you're short on time, just give me a thumbs up. Keen to get this in so the subinterp work in #1026 can start moving. |
|
@Siyet feel free to push. Otherwise I could get it done some time later this week |
provinzkraut
force-pushed
the
multi-phase-init
branch
from
4a5d5a1 to
d396f44
Compare
| state is populated in _core_exec and cleared and freed by m_clear and m_free | ||
| respectively. | ||
| */ | ||
| static MsgspecState *_core_state = NULL; |
There was a problem hiding this comment.
I would advokate against this pattern :)
It would be required to refactor it anyway for future FT / SI builds.
Maybe we can instead use something like
static MsgspecState *
_get_current_module_state(void)
{
PyObject *mod = _get_current_module();
if (mod == NULL) {
mod = PyImport_ImportModule("msgspec._core");
if (mod == NULL) {
return NULL;
}
}
MsgspecState *state = get_module_state(mod);
Py_DECREF(mod);
return state;
}?
There was a problem hiding this comment.
Or PyType_GetModuleByDef / PyType_GetModule :)
There was a problem hiding this comment.
I've tried using PyType_GetModule at first, but there were some issues. Can't remember what it was though, it's been a while. Let me re-evaluate that again
| .m_methods = msgspec_methods, | ||
| .m_traverse = msgspec_traverse, | ||
| .m_clear = msgspec_clear, | ||
| .m_free =(freefunc)msgspec_free, |
There was a problem hiding this comment.
| .m_free =(freefunc)msgspec_free, | |
| .m_free = (freefunc)msgspec_free, |
style nit
| #ifdef Py_GIL_DISABLED | ||
| PyUnstable_Module_SetGIL(m, Py_MOD_GIL_NOT_USED); | ||
| #if !PY313_PLUS | ||
| PyUnstable_Module_SetGIL(m, Py_MOD_GIL_NOT_USED); |
There was a problem hiding this comment.
This is not needed, we set {Py_mod_gil, Py_MOD_GIL_NOT_USED}, module slot
| if (st->astimezone == NULL) return -1; | ||
|
|
||
| /* uuid module imports */ | ||
| temp_module = PyImport_ImportModule("uuid"); |
There was a problem hiding this comment.
There are missing Py_DECREF(temp_module) calls in many places.
There was a problem hiding this comment.
Yeah, I think this was discussed in a previous PR. I think we should clean those up. Should be a separate PR though IMO
4 participants
Closes #563.
Implement multi-phase init.
Relatively straightforward, the only slight hiccough is
msgspec_get_global_state, which relied onPyState_FindModule/PyState_AddModule, which in turn don't work (well) with multi-phase init. I've replaced them with a globalstatic MsgspecState, set during module init; This is not safe for sub-interpreters, but handling it properly would be a large refactor that I think is best kept for separate PRs.Road to subinterpreter support
This is the first step for msgspec to support subinterpreters, but it's still a long way to go. I'll open a separate tracking issue for that once this gets merged, but it'll likely involve some larger refactors, especially around moving things to heap types where absolutely necessary, and ensuring this does not impact performance negatively.