Skip to content

Avoid calculating union in spread if property types are identical - #53413

Merged
Jake Bailey (jakebailey) merged 2 commits into
microsoft:mainfrom
jakebailey:fix-52345-4
Mar 21, 2023
Merged

Avoid calculating union in spread if property types are identical#53413
Jake Bailey (jakebailey) merged 2 commits into
microsoft:mainfrom
jakebailey:fix-52345-4

Conversation

@jakebailey

@jakebaileyJake Bailey (jakebailey) commented Mar 21, 2023

Copy link
Copy Markdown
Member

For #52345

When spreading, we remove undefined from the right type and union it with the left. If the two types are huge but overlap, we'll add all of the union's members together into one big list, then run subtype reduction over it, which for this test takes a long time. But, we can skip that work if we can recognize that left/right are the same. With #53406, this eliminates all of the bottlenecks in #52345.

Since this spread stuff is tricky, I generated a test which tests all combos of spreading + optional + missing to verify that I don't screw it up.

Likely, there's something more general that could be done to getUnionType here; it always recurses into every union and then does subtype reduction. I wonder if we can deduplicate the list first (ignoring undefined-ness), and trivially bail out early where possible, but that seemed a little tricky.

@typescript-botTypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 21, 2023
@jakebaileyJake Bailey (jakebailey) changed the title Avoid calculating union in spread if types are identicalAvoid calculating union in spread if property types are identicalMar 21, 2023
@jakebailey

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at 958017d. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the abridged perf test suite on this PR at 958017d. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at 958017d. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at 958017d. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the extended test suite on this PR at 958017d. You can monitor the build here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing main and refs/pull/53413/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..53413

Metricmain53413DeltaBestWorstp-value
Angular - node (v16.17.1, x64)
Memory used361,392k (± 0.01%)361,422k (± 0.01%)~361,362k361,465kp=0.199 n=6
Parse Time3.54s (± 0.60%)3.52s (± 0.91%)~3.49s3.56sp=0.466 n=6
Bind Time1.18s (± 0.71%)1.18s (± 0.46%)~1.18s1.19sp=0.855 n=6
Check Time9.49s (± 0.44%)9.54s (± 0.37%)~9.50s9.60sp=0.064 n=6
Emit Time7.92s (± 0.37%)7.97s (± 0.71%)~7.92s8.07sp=0.063 n=6
Total Time22.13s (± 0.22%)22.22s (± 0.30%)+0.10s (+ 0.44%)22.13s22.33sp=0.024 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used192,532k (± 0.03%)192,515k (± 0.02%)~192,463k192,582kp=0.873 n=6
Parse Time1.60s (± 0.85%)1.59s (± 1.08%)~1.56s1.61sp=0.317 n=6
Bind Time0.83s (± 0.62%)0.82s (± 0.00%)-0.01s (- 0.81%)0.82s0.82sp=0.025 n=6
Check Time10.18s (± 0.45%)10.11s (± 0.67%)-0.07s (- 0.72%)10.06s10.24sp=0.043 n=6
Emit Time2.99s (± 1.21%)3.00s (± 1.02%)~2.96s3.04sp=0.809 n=6
Total Time15.60s (± 0.30%)15.52s (± 0.64%)~15.43s15.69sp=0.126 n=6
Monaco - node (v16.17.1, x64)
Memory used345,562k (± 0.01%)345,556k (± 0.01%)~345,538k345,582kp=0.378 n=6
Parse Time2.73s (± 0.50%)2.73s (± 0.51%)~2.71s2.74sp=0.935 n=6
Bind Time1.09s (± 0.47%)1.09s (± 0.47%)~1.08s1.09sp=1.000 n=6
Check Time7.76s (± 0.52%)7.75s (± 0.57%)~7.68s7.81sp=0.808 n=6
Emit Time4.48s (± 0.58%)4.46s (± 1.05%)~4.43s4.55sp=0.290 n=6
Total Time16.06s (± 0.35%)16.02s (± 0.52%)~15.93s16.16sp=0.377 n=6
TFS - node (v16.17.1, x64)
Memory used299,778k (± 0.01%)299,816k (± 0.01%)~299,754k299,863kp=0.173 n=6
Parse Time2.17s (± 0.86%)2.16s (± 0.48%)~2.14s2.17sp=0.102 n=6
Bind Time1.23s (± 1.03%)1.24s (± 1.14%)~1.22s1.26sp=0.249 n=6
Check Time7.20s (± 0.39%)7.17s (± 0.44%)~7.12s7.20sp=0.107 n=6
Emit Time4.34s (± 0.50%)4.33s (± 0.60%)~4.30s4.37sp=0.331 n=6
Total Time14.93s (± 0.32%)14.89s (± 0.28%)~14.84s14.96sp=0.173 n=6
material-ui - node (v16.17.1, x64)
Memory used476,489k (± 0.02%)476,371k (± 0.00%)-118k (- 0.02%)476,351k476,391kp=0.013 n=6
Parse Time3.22s (± 1.21%)3.22s (± 0.32%)~3.20s3.23sp=0.366 n=6
Bind Time0.98s (± 3.78%)0.95s (± 0.57%)-0.03s (- 2.72%)0.95s0.96sp=0.038 n=6
Check Time18.00s (± 1.06%)17.93s (± 0.40%)~17.84s18.03sp=0.688 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time22.20s (± 1.03%)22.10s (± 0.31%)~22.02s22.18sp=0.689 n=6
xstate - node (v16.17.1, x64)
Memory used550,558k (± 0.01%)550,529k (± 0.01%)~550,489k550,600kp=0.471 n=6
Parse Time3.95s (± 0.34%)3.95s (± 0.23%)~3.94s3.96sp=0.681 n=6
Bind Time1.79s (± 0.47%)1.79s (± 0.58%)~1.78s1.81sp=1.000 n=6
Check Time2.99s (± 0.47%)2.99s (± 0.91%)~2.96s3.02sp=0.513 n=6
Emit Time0.09s (± 0.00%)0.09s (± 0.00%)~0.09s0.09sp=1.000 n=6
Total Time8.83s (± 0.34%)8.83s (± 0.37%)~8.79s8.87sp=0.747 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
BenchmarkNameIterations
Current534136
Baselinemain6

Developer Information:

Download Benchmark

@weswighamWesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably build a similar check into getUnionType directly, but the undefined handling (as done here) does make it a bit tricker.

@typescript-botTypeScript Bot (typescript-bot) added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 21, 2023
@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing main and refs/pull/53413/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@jakebailey

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) run dt but this time don't fail please

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at 958017d. You can monitor the build here.

Update: The results are in!

@jakebailey

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at 958017d. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/150524/artifacts?artifactName=tgz&fileId=500A00E3BCD5C97B02ABAC8526FB418BC7F239F8EF69AF70BD2D56952FEE16B702&fileName=/typescript-5.1.0-insiders.20230321.tgz"
}
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53413-14".;

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@jakebailey
Jake Bailey (jakebailey) merged commit 01de788 into microsoft:mainMar 21, 2023
@jakebailey
Jake Bailey (jakebailey) deleted the fix-52345-4 branch March 21, 2023 21:44
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jakebailey@typescript-bot@weswigham