add second lifetime to `PyRef` and `PyRefMut`

[Icxolu]

Companion to #4390

[davidhewitt]

Thank you for making progress on this one. There's a measurable perf boost in the ordering operator benchmarks and I guess similarly this is a win for most #[pymethods], so it seems to me that we definitely do want to remove that ref counting op from PyRef.

Again the same thing that's paining me as in #4390 is I'm trying to think hard how we keep the API easy for users. Lifetimes scare Rust newbies enough, let alone two 😂

Are there any tricks we can play which avoid the complexity for the common case? Here's two I can think of:

• We could add the double lifetime type asPyRefBorrowed and make it deref to PyRef, similar to how we have Borrowed and Bound set up. We could use the double lifetime type mostly internally. Though I think really this isn't any good, because the truth is that users should still be using PyRefBorrwed for best performance and we just duplicate the API. So it's just a hot mess and not a real option.

• More promising, slightly crazy idea: can we change the meaning of the lifetime from PyRef<'py, T> to PyRef<'a, T>? I think with the changes to the borrow flag to be an atomic in 0.23 there's no technical dependency within PyRef of the Python attachment any more (was previously necessary for the Drop impl). I haven't actually investigated this but the only API standing in our way might be PyRef::py()? If so, maybe we can just deprecate that API for now and then in PyO3 0.26 we could switch the lifetime.

(Given that we're wanting to revisit pyclass borrow checking and locking anyway, maybe we could add new types PyClassGuard<'a, T> and PyClassGuardMut<'a, T> somewhat inspired by mutex guards. And we could just deprecate PyRef / PyRefMut to get the result we want immediately.)

Opinions welcome to which, if any, of these ideas are not crazy 😂

[Icxolu]

Thanks for your thoughts!

Again the same thing that's paining me as in #4390 is I'm trying to think hard how we keep the API easy for users. Lifetimes scare Rust newbies enough, let alone two 😂

That's certainly true.

I agree that introducing PyRefBorrowed doesn't really lead anywhere and would mainly duplicating the API without bringing a real benefit.

The second option sounds very interesting. Switching the lifetime and introducing it via PyClassGuard<'a, T> and PyClassGuardMut<'a, T> could make an easier migration than breaking PyRef/PyRefMut. This makes me wonder if something like this could also apply for FromPyObject. I think most types don't make use of the 'py lifetime, they either have no lifetime or need 'a.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #4720 will not alter performance

_{Comparing Icxolu:pyref (9abc0f0) with main (a95eecd)}

:tada: Hooray! codspeed-rust just leveled up to 2.10.1!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability :partying_face:! Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 88 untouched benchmarks

[davidhewitt]

The second option sounds very interesting. Switching the lifetime and introducing it via PyClassGuard<'a, T> and PyClassGuardMut<'a, T> could make an easier migration than breaking PyRef/PyRefMut. This makes me wonder if something like this could also apply for FromPyObject. I think most types don't make use of the 'py lifetime, they either have no lifetime or need 'a.

I feel like I wrote a reply to this ages ago, but it doesn't look like it's here, so probably it got interrupted by family (sorry both for that and the long long lag to actually reply here).

I think the difference in FromPyObject is that Bound<'py, T> and Borrowed<'a, 'py, T> definitely do need the 'py lifetime and they are pretty core to the whole system.

... but maybe if we accept two traits, the second trait becomes FromPyObjectAttached<'a, 'py, T>? That is at least a new angle for us to experiment with!

[davidhewitt]

As for this here, it seems then that the migration path we quite like here is to add new PyClassGuard types.

We can probably introduce those already and implement FromPyObjectBound for them, so it might be the case that sorting out these types and unlocking the perf win doesn't need to get blocked on us figuring out what to do with FromPyObject.

[davidhewitt]

... but maybe if we accept two traits, the second trait becomes FromPyObjectAttached<'a, 'py, T>? That is at least a new angle for us to experiment with!

FromPyObjectBound<'a, 'py, T> which we already have may also not be a terrible name for such a type.

As a final thought, I still have a vague hope that in the long run we might be able to make the Rust compiler use the unicorn feature "contexts" to make Py<T> and Borrowed<'a, T> the only types we need, and we could drop Bound<'py, T>. It seems quite cute that if FromPyObject only had the 'a lifetime it would probably fit that future dream quite well.

[Icxolu]

I feel like I wrote a reply to this ages ago, but it doesn't look like it's here, so probably it got interrupted by family (sorry both for that and the long long lag to actually reply here).

No worries 😄 This is kind of a big change with lots of paths that we can take. So I expected it to take time to figure out the design.

As for this here, it seems then that the migration path we quite like here is to add new PyClassGuard types.

I started an experiment with those in #5233. Looks promising, I'll continue playing with it.

[davidhewitt]

Shall we close this in favour of #5233 ?

[Icxolu]

I think so, yes.