[experiment] Sort intersections #52891
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 9dec535. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - main..52891
System
Hosts
Scenarios
TSServerComparison Report - main..52891
System
Hosts
Scenarios
StartupComparison Report - main..52891
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Nice job finding other possible wins here - the "fast path" I described in our chat was actually in That function has some code that can now take advantage of intersections being sorted as well: - if (source.flags & TypeFlags.Union && containsType(sourceTypes, target)) {
+ if (containsType(sourceTypes, target)) {
return Ternary.True;
} |
|
The question is: is the sorting itself a perf issue? I recall it being mentioned that perf was the primary reason unions aren't sorted, though to be fair intersections are much less common than unions. Also wasn't there an issue where stuff like |
|
To the former point, I would think unions are more common and therefore the fact that this PR doesn't seem to cause a massive regression is a good sign. I haven't gone through and "enabled" all of the other wins. For the latter, yes, I need to figure that out. But, all of the tests (except one) fail just because this PR doesn't have the origin type thing to preserve the "looks" of intersections. So I'm hopeful I can make everything consistent. |
Yep, hence my "to be fair" caveat. |
jakebailey
commented
Feb 21, 2023
src/compiler/checker.ts
Outdated
| // TODO(jakebailey): we already have insertSorted and should use it here (adding the perf optimization?) | ||
| const len = types.length; | ||
| // Perf optimization (measure?); check the last element first as we often insert in order. | ||
| const index = len && type.id > types[len - 1].id ? ~len : binarySearch(types, type, getTypeId, compareValues); |
There was a problem hiding this comment.
I tested this perf optimization specifically and I don't think it actually helps...
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 56c9459. You can monitor the build here. Update: The results are in! |
|
|
|
Sweet! I'll also mention - even if you don't end up sorting intersection types by their IDs due to other concerns, you can still add a linear walk over every type. That'll provide earlier bail-outs without having to deeply explore every preceding type. |
Oh, that's a good idea. I'll have to try that for #51188. We have to go over the intersection's types anyway so another linear iteration seems fine. |
|
Ugh nevermind I screwed up; I had commented out the slow code in my example... There's still a benefit, but it goes from 40s to 30s. I mean, still good. |
|
I will happily take a 25% speed-up 😄 Still, sounds like we can do better for that issue. |
|
Hm, not sure where my 40s number came from anymore, because now it's 30 without the last commit too. I'm going to stop giving updates before I make myself look any sillier. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52891
System
Hosts
Scenarios
TSServerComparison Report - main..52891
System
Hosts
Scenarios
StartupComparison Report - main..52891
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yeah, this is the diff I expected; the fast paths mentioned aren't actually that fast because isRelatedTo already checks equality. But still, the perf benchmarks appear to indicate that this sorting itself isn't catastrophic, so I'm going to continue with it and get origin types working. |
|
My hopes for this went away via standup; I completely forgot that intersection ordering determines overload order, and therefore we can't sort intersections at all. |
|
@typescript-bot test it for fun |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Changes are too big to display here, please check the log. |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
|
fun is over, it's not really faster |
|
@jakebailey Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #51188, doing this is the difference between OOMing and just taking "too long", because doing this allows us to reuse intersections, compare using fast paths, etc, when producing intersections which are the same but just happen to be in different orders.
To actually do this, we'd need to add
originto intersections too (hence, test failures), but I want to first see what the perf tests say.