store Bound<T> inside PyRef and PyRefMut#3860
Conversation
| } | ||
|
|
||
| fn get_cell(&'py self) -> &'py PyCell<T> { | ||
| pub(crate) fn get_cell(&'py self) -> &'py PyCell<T> { |
There was a problem hiding this comment.
Does this need to be &'py self or could it be &self?
There was a problem hiding this comment.
Ugh, so it could probably just be &self and things would work but the problem is that because PyCell<T> is a GIL Ref, I want to respect that still and keep the 'py very formal for now. This is exactly part of what I mean by the implementation is a bit messy.
I think that to solve this we might need to split PyCell into the public GIL Ref type, and a private inner PyCell (or will a different name like PyCellObject) and change all this stuff to work off the private inner type. But I'm not completely decided on that.
CodSpeed Performance ReportMerging #3860 will not alter performanceComparing Summary
|
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
I've played around with this a bit yesterday. I agree with you that we would need the second lifetime to avoid the ownership requirement. That's a bit sad, but I guess it has to do for now.
|
Thanks @LilyFoote @Icxolu for the reviews. I've finally come back to this and tidied it up. I stalled for a bit both because I was busy and because I was wondering about going ahead with the greater refactor of splitting For now I left that; it won't block us getting 0.21 shipped and would be a great tidy up to do on the way to 0.22, perhaps. |
LilyFirefly
left a comment
LilyFirefly
left a comment
There was a problem hiding this comment.
Still looks good to me!
|
🎉 thanks both! |
3 participants
Part of #3684, split from #3606
This first commit in this PR rewrites the internals of
PyRefandPyRefMutso that they storeBound<T>instead of the gil-ref&PyCell<T>. This incurs a necessary reference counting overhead where there wasn't before.This is necessary step to get lifetimes to wire up successfully for
FromPyObjectimplementations forPyRefandPyRefMutto use theBoundAPIs. Inextract_bound, while we only have the one lifetime onFromPyObject(for compatibility reasons) we cannot borrow from the input&Bound. So we are forced to take ownership of a new reference. This could be removed again in future when we add the lifetime toFromPyObject.The second commit makes the changes to the
FromPyObjectimplementations, which required loosening of lifetimes onBound::borrowand similar methods.Overall I don't love where this gets to in terms of implementation, so suggestions are welcome and I'll be glad to clean this up as soon as there's a better solution.