Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Speed up make_simplified_union, fix recursive tuple crash#15128

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
hauntsaninja merged 8 commits intopython:masterfromhauntsaninja:msu
May 6, 2023

Conversation

@hauntsaninja
Copy link
Collaborator

@hauntsaninjahauntsaninja commentedApr 25, 2023•
edited
Loading

Fixes#15192

The following code optimises make_simplified_union in the common case that there are exact duplicates in the union. In this regard, this is similar to#15104

There's a behaviour change in one unit test. I think it's good? We'll see what mypy_primer has to say.

To get this to work, I needed to use partial tuple fallbacks in a couple places (these maybe had the potential to be latent crashes anyway?) There were some interesting things going on with recursive type aliases and type state assumptions

This is about a 25% speedup on the pydantic codebase and about a 2% speedup on self check (measured with uncompiled mypy)

adriangb reacted with rocket emoji
hauntsaninjaand others added2 commitsApril 25, 2023 10:18
The following code optimises make_simplified_union in the common casethat there are exact duplicates in the union. In this regard, this issimilar topython#15104To get this to work, I needed to use partial tuple fallbacks in a coupleplaces (these maybe had the potential to be latent crashes anyway?)There were some interesting things going on with recursive type aliasesand type state assumptionsThis is about a 25% speedup on the pydantic codebase and about a 2%speedup on self check (measured with uncompiled mypy)
@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
CollaboratorAuthor

hauntsaninja commentedApr 25, 2023•
edited
Loading

Looks like there's a major performance regression inmaterialize

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff frommypy_primer, showing the effect of this PR on open source code:

pandas-stubs (https://github.com/pandas-dev/pandas-stubs) got 1.14 faster (68.0s -> 59.5s)speedrun.com_global_scoreboard_webapp (https://github.com/Avasam/speedrun.com_global_scoreboard_webapp) got 1.24 faster (43.8s -> 35.2s)
AlexWaygood and ichard26 reacted with hooray emoji

@hauntsaninjahauntsaninja marked this pull request as ready for reviewApril 26, 2023 00:11
@hauntsaninja
Copy link
CollaboratorAuthor

(note to self: once this is merged, renameis_simple_literal tois_simple_str_literal or see if we can get rid of it)

@JukkaL
Copy link
Collaborator

I did a measurement usingmisc/perf-compare.py and this seems roughly performance-neutral for self check using a compiled mypy:

=== Results ===hauntsaninja-msu          4.393s (0.0%)master                    4.405s (+0.3%)

Copy link
Collaborator

@JukkaLJukkaL 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! I'm happy to see additional performance wins. I finally got around to reviewing this now that my jet lag is better.

Left a few additional ideas (optional, since this already seems like a nice improvement).

# If deleted subtypes had more general truthiness, use that
orig_item=new_items[duplicate_index]
ifnotorig_item.can_be_trueandti.can_be_true:
new_items[duplicate_index]=true_or_false(orig_item)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we only changecan_be_true, or is it ok to only adjustcan_be_false?

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

This is a good question. I'm going to preserve the existing behaviour for now and explore this in another PR. These code paths don't have many unit tests (see#15094 and#15098), so I want to be careful here

ifnotorig_item.can_be_trueandti.can_be_true:
new_items[duplicate_index]=true_or_false(orig_item)
elifnotorig_item.can_be_falseandti.can_be_false:
new_items[duplicate_index]=true_or_false(orig_item)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to above.

@hauntsaninjahauntsaninja changed the titleSpeed up make_simplified_union, remove a potential crashSpeed up make_simplified_union, fix a crashMay 5, 2023
@hauntsaninja
Copy link
CollaboratorAuthor

hauntsaninja commentedMay 5, 2023•
edited
Loading

This fixes a crash that just got reported#15192, so I added a regression test for it.

I added the two micro optimisations suggested.

@github-actions
Copy link
Contributor

Diff frommypy_primer, showing the effect of this PR on open source code:

pandas-stubs (https://github.com/pandas-dev/pandas-stubs) got 1.15x faster (66.6s -> 58.0s)speedrun.com_global_scoreboard_webapp (https://github.com/Avasam/speedrun.com_global_scoreboard_webapp) got 1.27x faster (33.3s -> 26.2s)

@hauntsaninjahauntsaninja changed the titleSpeed up make_simplified_union, fix a crashSpeed up make_simplified_union, fix recursive tuple crashMay 6, 2023
@hauntsaninjahauntsaninja merged commit7832e1f intopython:masterMay 6, 2023
@hauntsaninjahauntsaninja deleted the msu branchMay 6, 2023 20:22
@hauntsaninja
Copy link
CollaboratorAuthor

hauntsaninja commentedMay 6, 2023•
edited
Loading

Nice, looks like the primer timings are pretty consistent between:
#15128 (comment)
#15128 (comment)
and some of the earlier commits on this PR (although those earlier commits had the regression on Materialize)

@ilevkivskyi
Copy link
Member

This PR actually introduced a recursive tuple crash

[case testAliasRecursiveUnpack]from typing import Tuple, TypeVar, OptionalT = TypeVar("T")S = TypeVar("S")A = Tuple[T, S, Optional[A[T, S]]]  # Must have two non-recursive items to crashx: A[int, str]*_, last = xif last is not None:    reveal_type(last)[builtins fixtures/tuple.pyi]

I will check maybe there is a simple fix for it.

@ilevkivskyi
Copy link
Member

It looks like there is (a bug is quite obvious). I will submit a PR shortly.

@ilevkivskyi
Copy link
Member

See#15216

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JukkaLJukkaLJukkaL left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segfault when manipulating a recursive union

3 participants

@hauntsaninja@JukkaL@ilevkivskyi
[8]ページ先頭
©2009-2025 Movatter.jp