Cache the regularized form of union types #37749
Merged
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 perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at d702b0d. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..37749
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn
approved these changes
Apr 2, 2020
| @@ -0,0 +1,8012 @@ | |||
| type BigUnion = | |||
| { | |||
| name: '0'; | |||
There was a problem hiding this comment.
how long does does the original 1,000 take without the fix? 30 seconds post-fix is still pretty long to wait for a test to run.
There was a problem hiding this comment.
It's actually timing out on CI because it's taking more than 40 seconds there. I may need to either trim it a bit, or try to see what else is doing poorly and improve the perf more. 40 seconds for a single file with 6 statements and no control flow is still unreasonably bad, just not as unreasonable.
|
Oops, I meant, why doesn't memory usage increase, not improve. |
ahejlsberg
approved these changes
Apr 2, 2020
Why would memory usage change? Unions are globally cached so long as their members are the same, so the final type counts are inevitably the same. All that's different is we now keep a direct reference to the result on the union (saving us from needing to iterate over it more than once), which means the only addition is a property, which are allocated by the vm in blocks (rather than one at a time) anyway. |
|
The VM allocating properties in blocks was the missing piece in my understanding. Thanks. |
ahejlsberg
reviewed
Apr 3, 2020
| const sourceType = sourceTypes[i]; | ||
| if (target.flags & TypeFlags.Union && (target as UnionType).types.length === sourceTypes.length) { | ||
| // many unions are mappings of one another; in such cases, simply comparing members at the same index can shortcut the comparison | ||
| const related = isRelatedTo(sourceType, (target as UnionType).types[i], /*reportErrors*/ false, /*headMessage*/ undefined, intersectionState); |
There was a problem hiding this comment.
Not sure I see why this would help. Union constituents are sorted by their type ids and those are pretty random even when one union is a mapping of the other. Am I missing something?
There was a problem hiding this comment.
Type IDs are assigned as they are made, and unions are sorted; ergo, unions are ordered by the order the types within them were created in - for a union that results from a mapping on another union, that means the resulting union will share the input union's order. We exploit this here by checking those corresponding elements against one another to make this common case linear in comparison time (when the result is assignable), rather than quadratic.
There was a problem hiding this comment.
This change is responsible for most of the performance improvements from the last commit, not the other one. The other one allows us to skip one silly quadratic comparison of a union to itself, while this is allowing us to compare BigUnion to DiscriminateType<BigUnion, "name", T> in a linear way (DiscriminateType is a distributive conditional, so produces a union of types which are ultimately identical to BigUnion, as T is constrained to the values of "name").
There was a problem hiding this comment.
Right, in cases where the mapping creates new type IDs for each element I can see how this helps.
|
So my last commit takes the test runtime locally down from 30s to 5s, but CI is failing on |
|
@typescript-bot perf test this At least travis is green 😾 - hopefully perf can install and run. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at cbf1b04. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..37749
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Nice improvement on material-ui check time! |
This was referenced Dec 9, 2020
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.
Fixes #37744
I doubled the example size for good measure. This simple change takes the test run down from 11 minutes to 30 seconds. That 30 seconds could likely still stand to be improved, but this fixes the obvious complexity issue.