Optimize checking involving large discriminated union types #42556
Conversation
This file contains 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 test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 216266b. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 216266b. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 216266b. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 216266b. You can monitor the build here. |
ahejlsberg
requested review from
amcasey,
DanielRosenwasser,
RyanCavanaugh and
weswigham
|
@ahejlsberg Here they are:Comparison Report - master..42556
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 9e8f24b. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 9e8f24b. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 9e8f24b. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 9e8f24b. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - master..42556
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at af9552d. You can monitor the build here. Update: The results are in! |
|
Yammer looks good 👍 |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 3d6dbf4. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 3d6dbf4. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 3d6dbf4. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 3d6dbf4. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - master..42556
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Latest commits introduce caching and discriminant checks in subtype reduction and remove the 100 constituent limit I was experimenting with. Reduction in check time for Compiler-with-unions is now at 32%! |
# Conflicts: # src/compiler/checker.ts
|
hello, i'd like to test this PR with typescript-eslint can i request a tgz? > @typescript-bot pack this |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at d282c70. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
| } | ||
| } | ||
| } | ||
| return count >= 10 && count * 2 >= types.length ? map : undefined; |
There was a problem hiding this comment.
To bring this in line with the comments above:
Suggested change
| return count >= 10 && count * 2 >= types.length ? map : undefined; | |
| return count < 10 || types.length < count * 2 ? undefined : map; |
| // Entries with duplicate keys have unknownType as the value. | ||
| function mapTypesByKeyProperty(types: Type[], name: __String) { | ||
| const map = new Map<TypeId, Type>(); | ||
| let count = 0; |
There was a problem hiding this comment.
Suggested change
| let count = 0; | |
| // The count of types with unique keys. | |
| let count = 0; |
src/compiler/checker.ts
Outdated
| const objectFlags = getObjectFlags(type); | ||
| if (objectFlags & ObjectFlags.ObjectLiteral) { | ||
| const props = getPropertiesOfObjectType(type); | ||
| const propTypes = map(props, p => getInternedType(getTypeOfSymbol(p))); |
There was a problem hiding this comment.
Does this cause us to be any more eager in resolving method types than before?
There was a problem hiding this comment.
No. Method types aren't marked with ObjectFlags.ArrayLiteral or ObjectFlags.ObjectLiteral so they're not explored and therefore not resolved.
src/compiler/checker.ts
Outdated
| * with references to a single one of those types. The process is applied recursively to properties of | ||
| * object literals and elements of array literals. | ||
| */ | ||
| function deduplicateObjectOrArrayLiteralTypes(types: Type[]) { |
There was a problem hiding this comment.
We had discussed issues around circular types in object literal methods/getters in a past design meeting - do those concerns not apply here?
There was a problem hiding this comment.
They do not. See my reply above.
|
@typescript-bot perf test this faster |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at a18aac2. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at a18aac2. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..42556
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here they are:Comparison Report - master..42556
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Last commit removes the deduplication code for array literals. With the subtype reduction optimizations, deduplication is less impactful and only affects very large array literals of object literals. The Monaco RWC project has one such array literal (58K+ lines) the check time for which drops from 0.4s to 0.2s when deduplicated. That's nice, but given the rarity of array literals of that size, I don't think the added complexity of the deduplication code is justified and I'm merging this PR without it. We can always add it in a separate PR if people feel otherwise. |
amcasey
added a commit
to amcasey/TypeScript
that referenced
this pull request
May 4, 2021
PR microsoft#42556 was a nice optimization that dramatically sped up comparisons of discriminated unions. Unfortunately, the cost of determining whether a union is discriminated can be prohibitively high. In particular, an internal team with a very large repo saw their type count double and their memory usage increase from 6GB to 9GB, breaking their build. This changes splits the difference by not trying to compute the property types of intersection types - a notoriously slow operation.
amcasey
added a commit
that referenced
this pull request
May 7, 2021
PR #42556 was a nice optimization that dramatically sped up comparisons of discriminated unions. Unfortunately, the cost of determining whether a union is discriminated can be prohibitively high. In particular, an internal team with a very large repo saw their type count double and their memory usage increase from 6GB to 9GB, breaking their build. This changes splits the difference by not trying to compute the property types of intersection types - a notoriously slow operation.
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.
This PR implements multiple optimizations:
When feasible, constituent maps that map from discriminant unit types to corresponding constituent types are constructed for large union types. These maps allow matching constituents to be quickly identified through simple lookups instead of expensive linear scans. Constituent maps are deemed feasible for unions with 10 or more constituents and less than 25% duplication in discriminant keys. The maps are purely a fast path optimization--all existing logic remains in place with unchanged semantics for cases where the maps don't apply.
The optimizations reduce the check time of the worst case examples from #42522 close to 10x (from 1.6s to 0.18s). Furthermore, the check times for the Compiler-with-unions and Monaco test cases are reduced by 32% and 6% respectively.
Fixes #42522.