-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] feat: call native __len__ when defined
#19934
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| r1 :: bit | ||
| r2 :: bool | ||
| L0: | ||
| r0 = C.__len__(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line should be f.__len__() instead of C.__len__(f) because neither C nor C.__len__ nor F is final
But maybe its fine because F isn't decorated with mypyc_attr(allow_interpreted_subclasses=True)? Seeking feedback on this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mypyc performs static analysis on class hierarchies, and if you aren't doing separate compilation (most tests don't use it), it can identify all possible subclasses and thus determine that F is effectively final, since there are no (can be no) subclasses.
JukkaL
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you measured that this improves performance? Method calls should already be optimized and call the native __len__ method if it's defined. The goal is that gen_method_call generates the fastest possible method call, so any further optimizations should likely go there instead of every place where we generate method calls.
| r1 :: bit | ||
| r2 :: bool | ||
| L0: | ||
| r0 = C.__len__(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mypyc performs static analysis on class hierarchies, and if you aren't doing separate compilation (most tests don't use it), it can identify all possible subclasses and thus determine that F is effectively final, since there are no (can be no) subclasses.
| r1 :: bit | ||
| r2 :: bool | ||
| L0: | ||
| r0 = c.__len__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a perfect world, should gen_method_call output the IR on the left or on the right?
I'll try to reimplement the logic from this PR in there and see what happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case they should do the same thing, but the original IR is more consistent with the intent of the code, since we aren't calling a static method, so the it would be better.
|
Okay, I misunderstood what the IR was representing and no longer think this PR is necessary. Closing so we can focus on the others. |
When a native class defines
__len__, we don't need to use python's dispatch for a vtable lookup. Instead, we can call the C function directly.