Opaque Object Layout (take 2) #5728
Conversation
Icxolu
added
CI-no-fail-fast
|
Sorry for not getting around to rebasing my PR 😞. I had it on my TODO list but I've had other things that got in the way. It's been a while since I implemented the original but I think I can remember the solution I ended up with had some benefits compared to the GAT approach. If you want to go ahead with this version then that's OK. Otherwise I can prioritise bringing my PR up to date. |
|
Nothing to be sorry about. Thanks a lot for all the work you already put in! I can take a look again and compare the approaches but at least initially this one felt easier to follow for me and looked like it needed fewer changes to adapt to the current state. If there advantages for one or the other approach we can of course evaluate that before we decide to go with one. |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Very nice, thank you for moving this forward!
Overall this seems very mergeable, with a bunch of comments / suggestions. If I read correctly, as implemented there's not yet a way for users to usefully inherit from unknown sized types, that'll come in a follow up?
| /// Gets the offset of the dictionary from the start of the object in bytes. | ||
| #[inline] | ||
| pub fn dict_offset<T: PyClass>() -> ffi::Py_ssize_t { | ||
| PyClassObject::<T>::dict_offset() | ||
| pub fn dict_offset<T: PyClass>() -> PyObjectOffset { | ||
| <T as PyClassImpl>::Layout::dict_offset() | ||
| } | ||
|
|
||
| /// Gets the offset of the weakref list from the start of the object in bytes. | ||
| #[inline] | ||
| pub fn weaklist_offset<T: PyClass>() -> ffi::Py_ssize_t { | ||
| PyClassObject::<T>::weaklist_offset() | ||
| pub fn weaklist_offset<T: PyClass>() -> PyObjectOffset { | ||
| <T as PyClassImpl>::Layout::weaklist_offset() | ||
| } |
There was a problem hiding this comment.
Slight aside, and doesn't block this PR, it would be well worth solving #4253 so that we can eventually phase this bit of complexity out.
| #[cfg(Py_3_12)] | ||
| impl<T: PyClassImpl> PyClassObjectBaseLayout<T> for PyVariableClassObject<T> | ||
| where | ||
| <T::BaseType as PyClassBaseType>::LayoutAsBase: PyClassObjectBaseLayout<T::BaseType>, |
There was a problem hiding this comment.
This implementation seems to be perfectly duplicated with the one for PyStaticClassObject, which seems to imply a refactoring opportunity exists IMO.
There was a problem hiding this comment.
Do you have something particular in mind?
There was a problem hiding this comment.
I took a quick look at this and I couldn't see a way to deduplicate, I guess let's not block this PR on it, we can refactor later as we learn more.
| #[repr(C)] | ||
| struct ExpectedLayout { | ||
| ob_base: ffi::PyObject, | ||
| contents: PyClassObjectContents<FrozenClass>, | ||
| } |
There was a problem hiding this comment.
Should this be using PyStaticClassObject?
There was a problem hiding this comment.
It is redefined here, because the fields of PyStaticClassObject are intentionally private, so we can't compute the offset here without loosening that.
|
Thanks for the review!
Yeah, I did not include anything that uses it in here to keep the diff as small as possible (which is also why the coverage is a bit low). In follow ups we can than start making use of the mechanism. I would probably look into exposing native type inheritance on abi3 next. Likely starting with exceptions, so maybe we can finally sunset |
This allows for variable sized base classes in future by using negative basicsize values.
Variable sized base classes require the use of relative offsets
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Looks great, thanks both!
| #[cfg(Py_3_12)] | ||
| impl<T: PyClassImpl> PyClassObjectBaseLayout<T> for PyVariableClassObject<T> | ||
| where | ||
| <T::BaseType as PyClassBaseType>::LayoutAsBase: PyClassObjectBaseLayout<T::BaseType>, |
There was a problem hiding this comment.
I took a quick look at this and I couldn't see a way to deduplicate, I guess let's not block this PR on it, we can refactor later as we learn more.
3 participants
This a split-off of the opaque object layout mechanism from #4678 by @mbway
Since we bumped MSRV several times after the initial implementation and GATs are now possible, I decided to base this of that initial implementation. I rebased it on current
main, resolved the conflict and focused on the mechanism only. Basic__init__support landed in #4951, so that is removed from here. Metaclass support (extendingPyType) is also removed here and left as a followup.I hope this makes this easier to review and we can base of further improvements to the inheritance machinery on this.