RFC: automatic garbage collector derives
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 16.2k
- Forks
- 1k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 66
Description
One of the things I want to see improved most about the current PyO3 implementation is our support for the Python GC. It is currently implemented entirely by hand by users via the __traverse__ and __clear__ functions in #[pymethods].
Both of these functions are both relatively simple to implement as they have quite well specified behaviour. Yet, we ask users to do this manually and worse, if users don't do this properly, it leads to memory leaks. My experience working on pydantic-core (with complex object trees) is that it's easy to get a __traverse__ or clear implementation wrong.
I think one major step we can do to improve this is to introduce a new (automatically-derivable) trait which defines the correct behaviour, and require all pyclasses to implement this trait. We can probably even go further and automatically derive it as part of #[pyclass] (maybe with an opt-opt #[pyclass(derive_gc = false)].
Historically, I have thought that we needed specialization to make this trait performant and easily usable. Given that specialization is unlikely to be a solution for the foreseeable future, I've been wondering what we can do without it.
I think the following trait definition might be sufficient to allow us to move forward without specialization.
/// Unsafe to implement because `traverse` is not allowed to execute arbitrary Python code
unsafe trait PyGcIntegration {
/// Helper constant which allows optimization by eliminating needless calls to sub-structures
///
/// Setting this to `false` will cause many implemenations to never call this type's implementation, which
/// will lead to memory leaks if set incorrectly.
const MAY_CONTAIN_CYCLES: bool;
fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError>;
// TBC: is the `&mut self` a problem? Seems necessary, but `frozen` pyclasses may not
// support mutable access. There _might_ be something we can do which automatically locks
// mutexes etc.
fn clear(&mut self);
}
Example derive
#[derive(PyGcIntegration)]
struct Foo {
a: A,
b: B,
}
unsafe impl PyGcIntegration for Foo {
const MAY_CONTAIN_CYCLES: bool = A::MAY_CONTAIN_CYCLES || B::MAY_CONTAIN_CYCLES;
fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
if A::MAY_CONTAIN_CYCLES {
self.a.traverse(visit)?;
}
if B::MAY_CONTAIN_CYCLES {
self.b.traverse(visit)?;
}
Ok(())
}
fn clear(&mut self, visit: PyVisit) {
if A::MAY_CONTAIN_CYCLES {
self.a.clear()
}
if B::MAY_CONTAIN_CYCLES {
self.b.clear()
}
}
}
I think clear might be "shallow" at the first level of mutability, e.g. for Option:
unsafe impl PyGcIntegration for Option<T> {
const MAY_CONTAIN_CYCLES: bool = T::MAY_CONTAIN_CYCLES;
fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
if T::MAY_CONTAIN_CYCLES && let Some(inner) = self {
inner.traverse(visit)?;
}
Ok(())
}
fn clear(&mut self, visit: PyVisit) {
// eliminate all state in the `Option`, but only if the values might be part of cycles
if T::MAY_CONTAIN_CYCLES {
*self = None;
}
}
}
... while I think there's a ton of semantics which need to be clarified, I'm curious to see if we can make this work and replace hand-written __traverse__ and __clear__ with reliable derived functionality.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing PyO3's current hand-written traverse and clear support in #[pymethods], then evaluate the proposed PyGcIntegration trait and derive behavior. Clarify the unresolved semantics around clear mutability, frozen pyclasses, cycle detection, and supported field types; done means the design is agreed before implementation begins.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, rust
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100