-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[mypyc] Fast tuple equality checks #9343
Conversation
The tests failures are due to the runtime error described in the issue. |
some performance numbers(running on my mac locally, very imprecise) On master
with this PR:
|
mypyc/irbuild/ll_builder.py
Outdated
# Cast to bool if necessary since most types uses comparison returning a object type | ||
# See generic_ops.py for more information | ||
if not is_bool_rprimitive(compare.type): | ||
comapre = self.coerce(compare, bool_rprimitive, line) |
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.
typo ("comapre")
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.
Thanks! But I've already addressed that in e34c025. The problem now is the Tuple[T, ...]
nested within a regular Tuple
@JukkaL I think it's ready for review now. |
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.
Looks good, but I have on question
mypyc/irbuild/ll_builder.py
Outdated
equal = True if op == '==' else False | ||
result = self.alloc_temp(bool_rprimitive) | ||
# empty tuples | ||
if (equal and len(lhs.type.types) == 0 and len(rhs.type.types) == 0): |
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.
Why a special case for just equality?
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.
Nice catch!
No need for this to be in this PR, but it would be pretty straightforward to do |
Sure, only need to do some minor modifications. But they appear much less frequently than these two ops, so probably it's OK to lower their priority IMO. |
mypyc/irbuild/ll_builder.py
Outdated
# Cast to bool if necessary since most types uses comparison returning a object type | ||
# See generic_ops.py for more information | ||
if not is_bool_rprimitive(compare.type): | ||
compare = self.coerce(compare, bool_rprimitive, line) |
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.
Coerce only does a type check. I think that the normal CPython semantics would be to allow arbitrary objects that have a truth value, i.e. it may be better to use bool_op
instead. Consider what happens if a user-defined class defines __eq__
that returns a non-bool value such as 1
or 'x'
.
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.
Looks good!
closes mypyc/mypyc#728