PyO3 / PyO3/pyo3

RFC: automatic garbage collector derives

Open
#5,663 5 comments 2 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.