Fix perf regression from #42556#43949
Merged
amcasey merged 1 commit intomicrosoft:masterfrom May 7, 2021
Merged
Conversation
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.
typescript-bot
added
Author: Team
For Uncommitted Bug
weswigham
approved these changes
May 4, 2021
Member
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
IIRC, the fail case is not using this fast path for identifying which union member to compare to, so this seems fine; though it's maybe worth noting that changes to the input program (namely some more comparisons between types) could make it potentially bloom to this size anyway, since it's not like we were manifesting anything that might not have been made manifest for other checks.
Member
Author
|
@weswigham I think you're pointing out that this will slow down some compilations (those that would have tugged on the properties anyway and were using discriminated unions), but only to the performance they were at before #42556. Did I get that right? |
Member
|
Yeah. |
Member
|
@typescript-bot perf test this |
Collaborator
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at abfa620. You can monitor the build here. Update: The results are in! |
Member
|
@ahejlsberg we'd like to land this for the RC in case you want to take a quick look. |
Collaborator
|
@DanielRosenwasser Here they are:Comparison Report - master..43949
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
approved these changes
May 7, 2021
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
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.
Fixes #