Skip to content
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

add second lifetime to PyRef and PyRefMut #4720

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Nov 19, 2024

Companion to #4390

@Icxolu Icxolu force-pushed the pyref branch 2 times, most recently from 8ad5fcd to 04fadee Compare November 19, 2024 18:40
@davidhewitt
Copy link
Member

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
Copy link
Contributor Author

Icxolu commented Nov 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants