use ffi::MemberGef for #[pyo3(get)] fields of Py<T>

[davidhewitt]

This is a bit of a mess and needs some more work, so probably best not to review it quite yet.

Inspired by what I found in #4247 (comment) this PR reworks the implementation of #[pyo3(get)] with a couple of goals in mind:

• use ffi::PyMemberDef for readonly Py<T> fields (on frozen types)
• for read/write fields we probably want to use ToPyObject where possible instead of IntoPy<Py<PyAny>>
• we should continue to use IntoPy<Py<PyAny>> + Clone as a fallback to existing behaviour I guess

This seems like a good thing to land for a couple of reasons:

• Avoiding Clone means that users are less likely to experience a niggle on upgrade with the py-clone feature
• The ffi::PyMemberDef is significantly more performant as per #[pyo3(get)] much slower than python attribute access  #4247 (comment)

I will continue with tidying this up when I get the chance; I think this is worth landing in 0.22

[davidhewitt]

Uff, this is more painful than I was hoping for. On Rust 1.77+ where we have stable std::mem::offset_of, the current implementation works beautifully.

The problem is that on versions predating this, the current design doesn't work. The "backport" in memoffset::offset_of isn't usable in const contexts for our PyClassObject<T> type, due to the existence of the UnsafeCell (we run into rust-lang/rust#80384).

I found a solution by deferring evaluation of memoffset::offset_of to runtime but keeping it resolved at compile type via use of a trait. Deferring the evaluation to runtime does have the horrible result of complicating the create_type_object & macro machinery, though :(

[davidhewitt]

Excluding docs and changelog additions, which I'll push later, I think this is now ready for review.

Despite the added complexity, this has reasonably clear benefits to me:

• The new implementation of #[pyo3(get)] benchmarks faster that main
• #[pyo3(get)] on fields containing Py<T> no longer requires the py-clone feature

It will be nice to simplify things again in future after MSRV 1.77 :(

e.g. consider this snippet inspired by #4247

use pyo3::prelude::*;

#[pymodule]
mod pyo3_scratch {
    use pyo3::prelude::*;

    #[pyclass]
    struct RustClass {
        #[pyo3(get)]
        value: i32,
        #[pyo3(get)]
        py_value: Py<PyAny>,
    }

    #[pymethods]
    impl RustClass {
        #[new]
        fn new(py: Python<'_>) -> Self {
            RustClass {
                value: 0,
                py_value: 0.into_py(py),
            }
        }
    }
}

Benchmarks on main:

Python:
0.021051380999779212
Rust:
0.045959195000250475
Rust (Py<PyAny>):
0.04677982700013672

Benchmarks on this branch:

Python:
0.02018206499997177
Rust:
0.0368609289998858
Rust (Py<PyAny>):
0.034424829999807116

With #[pyclass(frozen)] we get the full optimization on the Py<T> field:

Python:
0.02090933299950848
Rust:
0.037850384000194026
Rust (Py<PyAny>):
0.020540618000268296

[mejrs]

The problem is that on versions predating this, the current design doesn't work. The "backport" in memoffset::offset_of isn't usable in const contexts for our PyClassObject<T> type, due to the existence of the UnsafeCell (we run into rust-lang/rust#80384).

Actually, the problem is that memoffset::offset_of is only usable in const in rust 1.65+, but msrv is 1.63

[davidhewitt]

Actually, the problem is that memoffset::offset_of is only usable in const in rust 1.65+, but msrv is 1.63

I thought this too, but I was running into trouble with memoffset::offset_of even on 1.79 🤔

[Icxolu]

• #[pyo3(get)] on fields containing Py<T> no longer requires the py-clone feature

Especially this one sounds really compelling. That's a big improvement in UX after the py-clone feature. But that's really some cursed wizardry 🧙 with the generics here 😅.

[davidhewitt]

• #[pyo3(get)] on fields containing Py<T> no longer requires the py-clone feature

Especially this one sounds really compelling. That's a big improvement in UX after the py-clone feature. But that's really some cursed wizardry 🧙 with the generics here 😅.

Haha, yes precisely when I realised this would be the way out of regressing UX with py-clone I was determined to get this in.

The generics... is indeed some dark magic. I think it's a combination of coffee and a bit of luck. I genuinely think I wouldn't have worked this out without stable offset_of! to get the initial version working in const contexts, as the extra complication with OffsetOf and deferring some of the resolution to runtime was a painful addition. I'll be glad when we can reverse that.

Let me know if there's any documentation or explanation I can push to help with review 👀

[mejrs]

I think I mostly understand this 😅

Can we duplicate the definition of PyClassObject but without the unsafecell, and use that to compute the offset? That should probably work on Rust 1.65+

Is there some way to test that the correct path is taken for various types? I'd like to be certain that this won't regress.

[davidhewitt]

Thanks for the review!

Can we duplicate the definition of PyClassObject but without the unsafecell, and use that to compute the offset? That should probably work on Rust 1.65+

I think that would indeed work for 1.65+. I guess without it working for MSRV I'm inclined to leave this as-is, because adding yet more complexity doesn't seem like an appealing option. Let's revisit when we get our next MSRV bump, it would be nice to remove the runtime calculation and switch to that mode 👍

Is there some way to test that the correct path is taken for various types? I'd like to be certain that this won't regress.

Good idea, I was being a bit lazy, added some unit tests to examine the generated items from the macro.

With those review adjustments made, I'm going to proceed to merge this so we are one step closer to the release.