Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

pyproto: remove inventory from implementation #1328

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Dec 19, 2020

This is a refactoring for #[pyproto] which uses dtolnay specialization instead of inventory as the underlying implementation.

The dtolnay specialization is done for a type PyClassProtocols<T: PyClass>. There are traits PyObjectProtocolSlots<T> etc. which are what get specialized. The default implementation is PyObjectProtocolSlots<T> for &'_ PyClassProtocols<T>, and the specialized implementation is PyObjectProtocolSlots<T> for PyClassProtocols<T>.

The traits with slot getter methods have been replaced by free functions to implement the slots, which the pyproto macro code directly uses.

It works out as slightly fewer LOC and also because all the protocols are known at compile time, the compiler should be able to optimise pyclass initialization better.

At the moment we lost the type checking we had with TypedSlot. If that's a concern I think I can reintroduce a type check in the slot defs in the macro code.

Follow-ups:

  • I think I can see a way to refactor the code further to implement Support all receiver types for all protocol methods #1206, but I'm not entirely sure it'll work out. So I stopped here for now which works nicely and I think is a nice simplification.
  • I think that we can make inventory an optional feature where it's only needed when users want multiple #[pymethods] blocks for a single PyClass. That'd be a nice simplification to remove overheads.

@davidhewitt davidhewitt force-pushed the pyproto-no-inventory branch 3 times, most recently from 12b4605 to e16bea5 Compare December 19, 2020 22:11
@kngwyu
Copy link
Member

kngwyu commented Dec 20, 2020

Awesome, thank you so much!
Though I don't think the inventory overhead is large, it seems good for me to have a simplified implementation.

The traits with slot getter methods have been replaced by free functions to implement the slots, which the pyproto macro code directly uses.

Aren't these changes actually unrelated to removing inventory? I like the way it flattens the definitions, though.

At a glance, this PR looks almost OK for me.
I feel that we can make things simpler in pyo3-derive-backend/src/defs.rs, but am not sure and need to check it again.

@davidhewitt
Copy link
Member Author

Aren't these changes actually unrelated to removing inventory?

Sort of - I wanted the new specialised traits to provide &'static [PyType_Slot] lists. I ran into various issues with const fn unless I flattened the getter methods away.

I feel that we can make things simpler in pyo3-derive-backend/src/defs.rs, but am not sure and need to check it again.

I think I can hard-code some simplifications about the paths in defs.rs - I'll push a commit to do so.

@davidhewitt
Copy link
Member Author

I pushed a commit which puts the module path on the top-level Proto def rather than each slot def. This reduces repetition and LOC a bit.

As I think these defs might have to change again to implement #1206 in the way I imagine, I think it's not worth doing further refactoring right now.

@davidhewitt
Copy link
Member Author

@kngwyu ok if I merge this today before the release? It technically "breaks" any code using the #[doc(hidden)] public implementation details of pyproto (though hopefully that's nobody), so it could be nicer to release as part of a major version.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I noted some additional suggestions.

@davidhewitt
Copy link
Member Author

👍 will merge this shortly, then rebase the release and push it out!

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

Successfully merging this pull request may close these issues.

2 participants