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

[mypyc] Fast tuple equality checks #9343

Merged
merged 12 commits into from Aug 27, 2020
Merged

[mypyc] Fast tuple equality checks #9343

merged 12 commits into from Aug 27, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie requested a review from JukkaL August 23, 2020 16:59
@TH3CHARLie TH3CHARLie marked this pull request as draft August 23, 2020 16:59
@TH3CHARLie
Copy link
Collaborator Author

The tests failures are due to the runtime error described in the issue.

@TH3CHARLie
Copy link
Collaborator Author

some performance numbers(running on my mac locally, very imprecise)

On master

interpreted: 0.180333s (avg of 5 iterations; stdev 0.61%)
compiled:    0.210294s (avg of 5 iterations; stdev 0.9%)

compiled is 0.858x faster

with this PR:

interpreted: 0.182221s (avg of 6 iterations; stdev 0.51%)
compiled:    0.055916s (avg of 6 iterations; stdev 0.35%)

compiled is 3.259x faster

@TH3CHARLie TH3CHARLie marked this pull request as ready for review August 24, 2020 12:52
@TH3CHARLie TH3CHARLie marked this pull request as draft August 24, 2020 13:30
# 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)
Copy link
Member

Choose a reason for hiding this comment

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

typo ("comapre")

Copy link
Collaborator Author

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

@TH3CHARLie TH3CHARLie marked this pull request as ready for review August 25, 2020 12:38
@TH3CHARLie
Copy link
Collaborator Author

@JukkaL I think it's ready for review now.

Copy link
Collaborator

@msullivan msullivan left a 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

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):
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch!

@msullivan
Copy link
Collaborator

No need for this to be in this PR, but it would be pretty straightforward to do < and friends too, right?

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Aug 25, 2020

No need for this to be in this PR, but it would be pretty straightforward to do < and friends too, right?

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.

# 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)
Copy link
Collaborator

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'.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good!

@JukkaL JukkaL merged commit a05f19e into python:master Aug 27, 2020
@TH3CHARLie TH3CHARLie deleted the tuple-equality branch August 27, 2020 10:45
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.

Make tuple equality faster
4 participants