-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ty] Improve UnionBuilder performance by changing Type::is_subtype_of calls to Type::is_redundant_with
#22337
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
fb77ea7 to
0567017
Compare
Merging this PR will improve performance by 17.32%Summary
Performance Changes
Comparing Footnotes
|
0567017 to
d1d8fd4
Compare
UnionBuilder performanceUnionBuilder performance by changing Type::is_subtype_of calls to Type::is_redundant_with
c5b12a3 to
6e97c3b
Compare
d1d8fd4 to
d5812ee
Compare
MichaReiser
left a comment
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.
I'd prefer if @carljm reviews this PR too because I'm not very familiar with the semantic differences of the two methods but this now matches the pattern we use in the more general push_type
d5812ee to
4952a0d
Compare
4952a0d to
d9f7d7a
Compare
carljm
left a comment
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!
It seems a bit subtle when we can use is_redundant_with and when we need is_subtype_of -- there are still several is_subtype_of checks here, one with enums and several when we are checking subsumption by a negated type. I wonder how we could improve this... but I don't think it needs to happen in this PR.
Pushing one doc-comment update.
I don't think our simplifications for intersections with negated gradual elements is quite right on |
Summary
This PR is stacked on top of #22339; review that one first.
Type::is_redundant_withhas mostly the same semantics asType::is_subtype_of, but:There are several places in the
UnionBuilderwhere it looks like we can safely usedType::is_redundant_withinstead ofType::is_subtype_of. This significantly improves performance, has no impact on our test suite, and (according to mypy_primer) has no impact on any ecosystem diagnostics or ty's memory usage. (The reported changes in diagnostics are all just our standard ecosystem flakes.)