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

more enum-related speedups #12032

Merged
merged 2 commits into from Apr 18, 2022
Merged

Conversation

huguesb
Copy link
Contributor

@huguesb huguesb commented Jan 21, 2022

As a followup to #9394 address a few more O(n**2) behaviors
caused by decomposing enums into unions of literals.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

I think it will be fair to say that I don't understand what is going on in this PR, but I hope to help you by fixing some minor things 😄

mypy/meet.py Outdated
return (
isinstance(x, Instance) and x.type.is_enum and
isinstance(y, UnionType) and
all(isinstance(z, LiteralType) and z.fallback.type == x.type # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need type: ignore here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because when mypy typechecks itself it complains about using isinstance on values of type Type, suggesting running them through get_proper_type first (which I don't think is appropriate here) or using this type: ignore

mypy/meet.py Outdated
return (
isinstance(x, LiteralType) and isinstance(y, UnionType) and any(
isinstance(z, LiteralType) and z == x # type: ignore[misc]
for z in y.items
Copy link
Member

Choose a reason for hiding this comment

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

.items or .relevant_items?

mypy/meet.py Outdated
# and crucially, we want to do that *fast* in case the enum is large
# so we do it before expanding variants below to avoid O(n**2) behavior
if (
is_enum_overlapping_union(left, right) or
Copy link
Member

Choose a reason for hiding this comment

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

style nit: we use \n or is_enum_overlapping_union(...

mypy/sametypes.py Outdated Show resolved Hide resolved
mypy/sametypes.py Outdated Show resolved Hide resolved
mypy/subtypes.py Outdated Show resolved Hide resolved
mypy/subtypes.py Outdated
# avoid redundant check for union of literals
for item in left.items:
if mypy.typeops.is_simple_literal(item):
assert isinstance(item, LiteralType) # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to declare item = get_proper_type(item)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or I suppose I might be able to use the new TypeGuard feature here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes 🙂

But, I am not sure that mypyc supports it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

black is compiled with mypyc and you added TypeGuard to it @sobolevn so it's at least worth a shot imo 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

well OK it turns out I misremembered sobolevn for someone else, but anyway the first half of my still stands 😅

mypy/subtypes.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@huguesb
Copy link
Contributor Author

huguesb commented Mar 2, 2022

Any chance to get this merged?

mypy/subtypes.py Show resolved Hide resolved
@@ -1137,6 +1150,17 @@ def report(*args: Any) -> None:
return applied


def try_restrict_literal_union(t: UnionType, s: Type) -> Optional[List[Type]]:
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a docstring to this function?

@huguesb huguesb mentioned this pull request Apr 14, 2022
As a followup to python#9394 address a few more O(n**2) behaviors
caused by decomposing enums into unions of literals.
@huguesb
Copy link
Contributor Author

huguesb commented Apr 17, 2022

Rebased on top of master after recent union-related changes

@github-actions

This comment has been minimized.

@JelleZijlstra JelleZijlstra self-requested a review April 17, 2022 14:28
mypy/meet.py Outdated
Comment on lines 146 to 148
all(x.type == p.fallback.type
for p in (get_proper_type(z) for z in y.relevant_items())
if isinstance(p, LiteralType))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
all(x.type == p.fallback.type
for p in (get_proper_type(z) for z in y.relevant_items())
if isinstance(p, LiteralType))
all(isinstance(p, LiteralType)) and x.type == p.fallback.type
for p in (get_proper_type(z) for z in y.relevant_items()))

Should it be this instead? As I understand it, this function checks that x is an enum and y is a union containing only members of x.

It would be helpful to add a docstring to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon looking at this again, I think the previous implementation was wrong since we only care about partial overlap. Fixed and documented accordingly.

@@ -153,14 +153,32 @@ def visit_literal_type(self, left: LiteralType) -> bool:

def visit_union_type(self, left: UnionType) -> bool:
if isinstance(self.right, UnionType):
# fast path for simple literals
def _extract_literals(u: UnionType) -> Tuple[Set[Type], List[Type]]:
Copy link
Member

Choose a reason for hiding this comment

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

I believe nested functions don't perform well with mypyc, can you make this a global function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mypy/subtypes.py Outdated
@@ -1199,6 +1213,18 @@ def report(*args: Any) -> None:
return applied


def try_restrict_literal_union(t: UnionType, s: Type) -> Optional[List[Type]]:
"""Helper function for restrict_subtype_away, allowing a fast path for Union of simple literals"""
Copy link
Member

Choose a reason for hiding this comment

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

This docstring doesn't say what the function does, which is the part I was having trouble with. What are the parameters and what does it return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra self-requested a review April 18, 2022 14:21
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications!

@JelleZijlstra JelleZijlstra merged commit 9cab296 into python:master Apr 18, 2022
@huguesb huguesb deleted the pr-enum-speedups branch April 20, 2022 04:18
JukkaL pushed a commit that referenced this pull request Apr 20, 2022
As a followup to #9394 address a few more O(n**2) behaviors
caused by decomposing enums into unions of literals.
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.

None yet

4 participants