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

Micro-optimize flatten_nested_unions #14368

Merged
merged 1 commit into from
Dec 29, 2022
Merged

Micro-optimize flatten_nested_unions #14368

merged 1 commit into from
Dec 29, 2022

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Dec 29, 2022

Avoid constructing a new list if there is nothing to flatten (and the input is a list, which if often the case).

@github-actions
Copy link
Contributor

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

Comment on lines +3387 to +3394
if not isinstance(types, list):
typelist = list(types)
else:
typelist = cast("list[Type]", types)

# Fast path: most of the time there is nothing to flatten
if not any(isinstance(t, (TypeAliasType, UnionType)) for t in typelist): # type: ignore[misc]
return typelist
Copy link
Member

Choose a reason for hiding this comment

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

I think we only need to convert it to a list if we're taking the fast path, correct?

Suggested change
if not isinstance(types, list):
typelist = list(types)
else:
typelist = cast("list[Type]", types)
# Fast path: most of the time there is nothing to flatten
if not any(isinstance(t, (TypeAliasType, UnionType)) for t in typelist): # type: ignore[misc]
return typelist
# Fast path: most of the time there is nothing to flatten
if not any(isinstance(t, (TypeAliasType, UnionType)) for t in types): # type: ignore[misc]
if not isinstance(types, list):
return list(types)
return cast("list[Type]", types)

Copy link
Collaborator Author

@JukkaL JukkaL Dec 29, 2022

Choose a reason for hiding this comment

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

I'm first converting it to a list since iterating over a list if faster than iterating over a sequence, and since we need to return a list anyway, so it would only delay the construction of the list. Since we take the fast path the vast majority of the time, the redundant construction of a list in the slow path is not a big deal, and we save some of the cost through faster iteration. If mypyc was smarter about iterating over sequences, your suggested approach would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation!

@JukkaL JukkaL merged commit 8884f7d into master Dec 29, 2022
@JukkaL JukkaL deleted the faster-flatten-unions branch December 29, 2022 13:42
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