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

Expressions use multiple inheritance for Constant, Variable, Operation #2396

Closed
wants to merge 1 commit into from

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Mar 1, 2021

More minimal PR after breaking it out of #1729

Removes some duplicate code

NOTE: This branch will be rebased onto master after expression-keyword-only-init branch/PR #2395 is merged

@ekilmer ekilmer marked this pull request as draft March 1, 2021 20:26
Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

I don't see any code problems with this PR, and I think the technique of dynamically creating __slots__ for multiply-inherited subclasses is really exciting. That said, since this more or less completely reverts #1635, I'd like to get your thoughts on a couple of matters:

First, changing the inheritance structure reduces the amount of duplicated code, but means we have to do things like the __xslots__ trick to get good performance. Are we certain the ergonomics of one approach are better than the other?

Second, how much does this change affect performance? #1635 gave us a modest improvement, but not large enough to be noticeable without measuring, so we should measure this branch as well to see how it stacks up.

@ekilmer ekilmer force-pushed the expression-keyword-only-init branch from 85f0fa2 to d65be27 Compare March 12, 2021 14:42
Base automatically changed from expression-keyword-only-init to master March 12, 2021 16:02
@ekilmer ekilmer force-pushed the expression-multi-inheritance branch from 45bd7b9 to 8135735 Compare March 12, 2021 21:44
@ekilmer
Copy link
Contributor Author

ekilmer commented Mar 23, 2021

After testing with multiple-styles, as was done in #1635 I saw a slight slowdown with the use of multiple inheritance. Testing with a microbenchmark that instantiates the Constant and Variable subclass objects also reveals some performance penalties for the multiple inheritance. I didn't test with Operation subclasses, but I suspect it to be a similar story and also more greatly contribute to a performance drop, since Operations are used more in expressions.

I think the test_Api test we have to do sanity checking is good enough, and I don't really expect too much churn on these objects to make maintenance unmanageable, so I will close this PR.

@ekilmer ekilmer closed this Mar 23, 2021
@ekilmer ekilmer deleted the expression-multi-inheritance branch April 4, 2022 16:17
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