-
Notifications
You must be signed in to change notification settings - Fork 899
add ModuleState, core lifecycle callbacks for #[pymodule]s
#5600
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
base: main
Are you sure you want to change the base?
Conversation
90fec38 to
d4360ed
Compare
|
got an ICE compiling chrono? https://github.com/PyO3/pyo3/actions/runs/19233273660/job/54976905090?pr=5600 That's new
|
715f9c9 to
8c05240
Compare
CodSpeed Performance ReportMerging #5600 will not alter performanceComparing Summary
|
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this seems like a reasonable step forward. The StateCapsule type is obviously completely empty, I'd prefer we had some tangible substance before merging.
I don't yet see the full picture of how they state type gets passed around various parts of the PyO3 user code, it would be helpful to understand the current direction and proposed APIs for users to write / read module state.
| /// Calling this function multiple times on a single ModuleState is a noop, | ||
| /// beyond the first | ||
| fn drop_impl(&mut self) { | ||
| if let Some(ptr) = self.inner.take().map(|state| state.as_ptr()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than Option<NonNull<_>> probably can use ManuallyDrop here.
The invariant "should not be accessed again" makes this sound like unsafe fn is appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this is a logic error rather than a safety one -- dropping the state without going through the m_free callback to drop the owning module can't result in memory safety issues, but will make our (and consumer) functions that expect a state to exist when it doesn't do the wrong thing. I'm happy to make it unsafe though it really shouldn't ever be called outside exactly the two cases in the code (Drop::drop & m_free callbacks)
Okay, can do. I'm currently using a typemap implementation that is similar to https://crates.io/crates/type-map but with more flexible bounds (the ability to have a Let me work on pushing up the typemap code plus what I have for the user API and we can talk about it from there |
This commit adds the core ModuleState & StateCapsule structs for interop with python's ModuleDef m_size field. We choose to keep the in-interpreter size small, only being a NonNull ptr to the actual (allocated by Rust) state. This is suboptimal from a pointer chasing perspective, because we'll eventually have interpreter->StateCapsule->TypeMap<T> dereference chains, but it makes it easier to implement and reason about.
This commit sets ModuleDef.m_size to size_of::<ModuleState>, and adds the relevant callback to ModuleDef.m_free. We also add C function defs for Py_mod_exec and ModuleDef.m_free callbacks. These will be used to initialize and free ModuleState in later patches. We do not yet set an initializer, as these are passed in m_slots, which are set from the `#[pymodule]` impl in pyo3-macros-backend.
Asserting we initialize ModuleState correctly, that it is accessible from both module_exec calls -- that is, during module initialization -- and from after the module is loaded.
8c05240 to
c09543c
Compare
This is ported from some of my internal code, with adjustments for edition = 2024. It was originally forked from rust-typemap but has diverged significantly, and been updated for newer rust versions. I freely relicense it under PyO3's licensing terms.
2613c9a to
6f5b849
Compare
Within we switch over to the real typemap backed StateMap implementation. We also add a marker trait, ModuleStateType which guarantees the properties we expect from stored StateMap types. Namely: Clone + Send (not Sync), though I'm not sure these are the right guarantees for python's memory model. Send is a requirement, as python can move PyModules between threads at will, and Clone makes dealing with ModuleState much easier longer term (speaking from experience with typemaps). I'm not sure if Sync should be a requirement too, as it's not clear to me if PyModules can be concurrently accessed between attached python thread states. I'm worried the answer might change between free-threaded and "normal" python builds.
These commit adds three methods to the external interface for PyModules, exposing a limited ability to interact with the per-module state area. - state_ref - state_mut - state_or_init We take a hands-off approach to what the concrete "shape" of this state is, beyond requiring it is compatible with the marker ModuleStateType trait. Modules may store as many types as needed, and all the normal rust visibility conventions apply, allowing them to guard type invariants, or prevent external access entirely.
So we can return a real error if our state ptr is null. The upstream cpython builtins just assert non-null, so this is probably fine References: https://github.com/python/cpython/blob/v3.13.0/Modules/_sqlite/module.h#L80-L81 References: https://github.com/python/cpython/blob/v3.13.0/Modules/_io/_iomodule.h#L172-L173 References: https://github.com/python/cpython/blob/v3.13.0/Modules/_blake2/blake2module.c#L33-L34
6f5b849 to
4075435
Compare
|
I've rebased, added the real statemap ( I think the next steps here are to have a discussion around the public API to module state, because I'm pretty sure it's wrong-ish. Maybe we can feature-gate it? I'm not sure how seriously pyo3 takes API stability, yet. |
This patch set adds the core internal representation for a
PyO3module's per-module state.Some of this work is taken from #4162 (credit: @Aequitosh), stripped down.
Within we...
repr(C)type that CPython will allocate (space for)PyModuleDeffor initialization (m_slotsPy_mod_exec slot) and freeing (m_free)TypeMapbased store for user state types and expose a limited API to this state via thePyModuleMethodstraitLeft for future work:
ModuleStatefor all modules, even though nothing uses it yet)PyModuleMethodsneeds work&mut selfvalid? I see every other method -- including those that do mutation (add_*) -- take&selfwhich makes me think this trait is expected handle synchronization internally