Skip to content

Bail early in getNarrowedTypeWorker if type is candidate - #55926

Merged
Jake Bailey (jakebailey) merged 2 commits into
microsoft:mainfrom
jakebailey:fix-52345-other
Nov 6, 2023
Merged

Bail early in getNarrowedTypeWorker if type is candidate#55926
Jake Bailey (jakebailey) merged 2 commits into
microsoft:mainfrom
jakebailey:fix-52345-other

Conversation

@jakebailey

@jakebaileyJake Bailey (jakebailey) commented Sep 30, 2023

Copy link
Copy Markdown
Member

This technically handles #52345 (comment) and maybe #55948, bringing it back to <=4.7 levels, but I think it's a little silly to narrow something to itself. But, probably cheap to check anyhow.

@jakebailey

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Sep 30, 2023

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Sep 30, 2023

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented Sep 30, 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/158040/artifacts?artifactName=tgz&fileId=5B641F9BC561DDCDFE93B4E7D8D9581096D3B6BE20D3E0ED9F4417909E1BA7EC02&fileName=/typescript-5.3.0-insiders.20230930.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.3.0-pr-55926-3".;

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Here they are:

Compiler

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,051k (± 0.02%)295,098k (± 0.01%)+48k (+ 0.02%)295,050k295,144kp=0.045 n=6
Parse Time2.63s (± 0.72%)2.64s (± 0.52%)~2.62s2.66sp=0.510 n=6
Bind Time0.84s (± 0.97%)0.84s (± 1.30%)~0.83s0.85sp=0.662 n=6
Check Time8.07s (± 0.13%)8.06s (± 0.20%)~8.03s8.08sp=0.249 n=6
Emit Time7.03s (± 0.48%)7.04s (± 0.30%)~7.02s7.07sp=0.418 n=6
Total Time18.57s (± 0.13%)18.57s (± 0.19%)~18.53s18.62sp=0.872 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used191,640k (± 1.23%)192,490k (± 1.20%)~190,658k196,458kp=0.936 n=6
Parse Time1.34s (± 0.78%)1.34s (± 0.99%)~1.32s1.35sp=0.451 n=6
Bind Time0.73s (± 0.00%)0.73s (± 0.00%)~0.73s0.73sp=1.000 n=6
Check Time9.21s (± 0.88%)9.27s (± 1.21%)~9.13s9.38sp=0.378 n=6
Emit Time2.63s (± 0.47%)2.63s (± 0.39%)~2.62s2.65sp=0.101 n=6
Total Time13.91s (± 0.56%)13.97s (± 0.87%)~13.81s14.11sp=0.378 n=6
Monaco - node (v18.15.0, x64)
Memory used347,296k (± 0.01%)347,319k (± 0.01%)~347,295k347,351kp=0.128 n=6
Parse Time2.46s (± 0.33%)2.45s (± 0.49%)~2.43s2.46sp=0.115 n=6
Bind Time0.94s (± 0.00%)0.94s (± 0.00%)~0.94s0.94sp=1.000 n=6
Check Time6.90s (± 0.28%)6.91s (± 0.57%)~6.84s6.95sp=0.418 n=6
Emit Time4.02s (± 0.26%)4.01s (± 0.55%)~3.99s4.05sp=0.100 n=6
Total Time14.33s (± 0.19%)14.31s (± 0.25%)~14.28s14.37sp=0.418 n=6
TFS - node (v18.15.0, x64)
Memory used302,549k (± 0.00%)302,538k (± 0.01%)~302,506k302,566kp=0.378 n=6
Parse Time2.00s (± 0.81%)2.02s (± 0.58%)~2.01s2.04sp=0.085 n=6
Bind Time1.01s (± 1.04%)1.00s (± 0.75%)~0.99s1.01sp=0.611 n=6
Check Time6.26s (± 0.47%)6.27s (± 0.50%)~6.23s6.31sp=0.418 n=6
Emit Time3.56s (± 0.70%)3.56s (± 0.14%)~3.56s3.57sp=0.360 n=6
Total Time12.83s (± 0.11%)12.86s (± 0.23%)~12.81s12.88sp=0.145 n=6
material-ui - node (v18.15.0, x64)
Memory used470,488k (± 0.00%)470,502k (± 0.00%)~470,480k470,518kp=0.230 n=6
Parse Time2.57s (± 0.38%)2.59s (± 0.80%)~2.57s2.62sp=0.181 n=6
Bind Time1.00s (± 0.52%)1.00s (± 1.37%)~0.98s1.02sp=0.794 n=6
Check Time16.60s (± 0.37%)16.58s (± 0.32%)~16.53s16.66sp=0.572 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.17s (± 0.35%)20.16s (± 0.20%)~20.11s20.21sp=0.687 n=6
xstate - node (v18.15.0, x64)
Memory used512,600k (± 0.02%)512,582k (± 0.01%)~512,498k512,618kp=0.689 n=6
Parse Time3.27s (± 0.36%)3.28s (± 0.32%)~3.26s3.29sp=0.406 n=6
Bind Time1.55s (± 0.26%)1.55s (± 0.26%)~1.54s1.55sp=1.000 n=6
Check Time2.87s (± 0.56%)2.89s (± 1.07%)~2.85s2.93sp=0.225 n=6
Emit Time0.08s (± 4.99%)0.08s (± 4.99%)~0.08s0.09sp=1.000 n=6
Total Time7.76s (± 0.23%)7.78s (± 0.38%)~7.73s7.82sp=0.145 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

tsserver

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,346ms (± 0.83%)2,343ms (± 1.10%)~2,313ms2,387msp=0.873 n=6
Req 2 - geterr5,366ms (± 1.40%)5,385ms (± 1.52%)~5,272ms5,455msp=0.873 n=6
Req 3 - references332ms (± 1.19%)327ms (± 0.16%)-5ms (- 1.61%)326ms327msp=0.039 n=6
Req 4 - navto275ms (± 0.82%)276ms (± 0.94%)~273ms280msp=0.935 n=6
Req 5 - completionInfo count1,356 (± 0.00%)1,356 (± 0.00%)~1,3561,356p=1.000 n=6
Req 5 - completionInfo82ms (±10.20%)84ms (± 8.27%)~75ms90msp=0.802 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,465ms (± 1.21%)2,482ms (± 0.76%)~2,456ms2,504msp=0.423 n=6
Req 2 - geterr4,155ms (± 1.59%)4,073ms (± 1.44%)-82ms (- 1.98%)4,028ms4,190msp=0.045 n=6
Req 3 - references335ms (± 1.03%)338ms (± 1.16%)~334ms343msp=0.089 n=6
Req 4 - navto283ms (± 0.27%)283ms (± 0.48%)~282ms285msp=1.000 n=6
Req 5 - completionInfo count1,518 (± 0.00%)1,518 (± 0.00%)~1,5181,518p=1.000 n=6
Req 5 - completionInfo78ms (± 5.82%)78ms (± 9.38%)~71ms87msp=0.801 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,595ms (± 0.38%)2,597ms (± 0.53%)~2,577ms2,618msp=1.000 n=6
Req 2 - geterr1,704ms (± 2.81%)1,676ms (± 1.97%)~1,631ms1,731msp=0.378 n=6
Req 3 - references109ms (± 4.57%)115ms (± 9.24%)~105ms126msp=0.468 n=6
Req 4 - navto360ms (± 0.41%)359ms (± 0.14%)-2ms (- 0.42%)358ms359msp=0.039 n=6
Req 5 - completionInfo count2,071 (± 0.00%)2,071 (± 0.00%)~2,0712,071p=1.000 n=6
Req 5 - completionInfo308ms (± 1.47%)307ms (± 1.61%)~303ms316msp=0.572 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Startup

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
tsc-startup - node (v18.15.0, x64)
Execution time152.43ms (± 0.17%)152.45ms (± 0.19%)~151.33ms156.11msp=1.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time227.01ms (± 0.15%)226.99ms (± 0.17%)~225.65ms232.33msp=0.294 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time228.69ms (± 0.16%)228.75ms (± 0.17%)~227.22ms235.65msp=0.191 n=600
typescript-startup - node (v18.15.0, x64)
Execution time228.51ms (± 0.16%)228.54ms (± 0.13%)~227.16ms231.65msp=0.124 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@jakebailey

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) perf test this faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the tsc-only perf test suite on this PR at 1c14f5c. You can monitor the build here.

Update: The results are in!

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Here they are:

Compiler

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,067k (± 0.01%)295,065k (± 0.01%)~295,049k295,089kp=0.936 n=6
Parse Time2.63s (± 0.52%)2.63s (± 0.44%)~2.61s2.64sp=0.865 n=6
Bind Time0.83s (± 0.62%)0.84s (± 0.97%)+0.01s (+ 1.20%)0.83s0.85sp=0.050 n=6
Check Time8.07s (± 0.38%)8.07s (± 0.39%)~8.03s8.12sp=0.627 n=6
Emit Time7.04s (± 0.33%)7.06s (± 0.12%)+0.02s (+ 0.31%)7.05s7.07sp=0.026 n=6
Total Time18.58s (± 0.10%)18.60s (± 0.18%)~18.56s18.66sp=0.413 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used193,048k (± 1.48%)191,115k (± 0.54%)~190,674k193,234kp=0.810 n=6
Parse Time1.35s (± 1.24%)1.35s (± 0.30%)~1.34s1.35sp=0.446 n=6
Bind Time0.73s (± 0.00%)0.73s (± 0.00%)~0.73s0.73sp=1.000 n=6
Check Time9.19s (± 0.70%)9.19s (± 0.54%)~9.14s9.26sp=1.000 n=6
Emit Time2.64s (± 0.65%)2.63s (± 0.34%)~2.62s2.64sp=0.458 n=6
Total Time13.90s (± 0.54%)13.90s (± 0.38%)~13.84s13.97sp=1.000 n=6
Monaco - node (v18.15.0, x64)
Memory used347,300k (± 0.00%)347,296k (± 0.00%)~347,284k347,309kp=0.575 n=6
Parse Time2.46s (± 0.40%)2.46s (± 0.21%)~2.45s2.46sp=0.931 n=6
Bind Time0.94s (± 0.43%)0.94s (± 0.43%)~0.94s0.95sp=0.218 n=6
Check Time6.91s (± 0.53%)6.91s (± 0.35%)~6.88s6.95sp=0.629 n=6
Emit Time4.02s (± 0.38%)4.01s (± 0.44%)~3.98s4.03sp=0.103 n=6
Total Time14.33s (± 0.33%)14.31s (± 0.25%)~14.26s14.37sp=0.419 n=6
TFS - node (v18.15.0, x64)
Memory used302,560k (± 0.01%)302,546k (± 0.00%)~302,527k302,566kp=0.748 n=6
Parse Time1.99s (± 0.61%)2.00s (± 0.88%)~1.98s2.03sp=0.277 n=6
Bind Time1.01s (± 1.02%)1.00s (± 1.22%)~0.99s1.02sp=0.784 n=6
Check Time6.27s (± 0.17%)6.27s (± 0.16%)~6.26s6.28sp=0.931 n=6
Emit Time3.57s (± 0.52%)3.55s (± 0.34%)~3.54s3.57sp=0.061 n=6
Total Time12.84s (± 0.18%)12.83s (± 0.15%)~12.80s12.86sp=0.285 n=6
material-ui - node (v18.15.0, x64)
Memory used470,512k (± 0.01%)470,506k (± 0.00%)~470,486k470,524kp=0.936 n=6
Parse Time2.58s (± 0.32%)2.58s (± 0.53%)~2.56s2.60sp=0.605 n=6
Bind Time0.99s (± 1.18%)0.99s (± 0.76%)~0.98s1.00sp=0.796 n=6
Check Time16.63s (± 0.26%)16.62s (± 0.40%)~16.54s16.69sp=0.688 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.20s (± 0.17%)20.19s (± 0.28%)~20.13s20.25sp=0.630 n=6
xstate - node (v18.15.0, x64)
Memory used512,583k (± 0.00%)512,609k (± 0.02%)~512,506k512,745kp=0.378 n=6
Parse Time3.27s (± 0.41%)3.27s (± 0.37%)~3.24s3.27sp=0.675 n=6
Bind Time1.55s (± 0.33%)1.55s (± 0.33%)~1.55s1.56sp=1.000 n=6
Check Time2.88s (± 0.64%)2.88s (± 0.99%)~2.84s2.92sp=0.871 n=6
Emit Time0.08s (± 0.00%)0.08s (± 4.99%)~0.08s0.09sp=0.405 n=6
Total Time7.78s (± 0.20%)7.77s (± 0.43%)~7.74s7.82sp=0.293 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@jakebailey

Copy link
Copy Markdown
MemberAuthor

I still feel like this might be slightly silly, but it seems really cheap and quick to just check for cases where we're trying to narrow something to itself and bail early without doing all of the other work.

@gabritto

Copy link
Copy Markdown
Member

The change makes sense, but where in type checking the motivating case https://github.com/epmatsw/tsdemo/blob/main/slow.ts do we call getNarrowedType with type and candidate being the same?

@jakebailey

Copy link
Copy Markdown
MemberAuthor

I think the example has been updated; it used to be narrowing something to itself.

@gabritto

Copy link
Copy Markdown
Member

I think the example has been updated; it used to be narrowing something to itself.

Do you have an example then?

@jakebailey

Copy link
Copy Markdown
MemberAuthor

The repro repo has it if you go back a few commits: https://github.com/epmatsw/tsdemo/tree/b2a2af937e492819045ea4e880a56236494c2147

Something like:

functiontest(base: SpecificString){if(isSpecificString(base)){returnbase;}}

Where SpecificString is a large union of literals.

@gabritto

Copy link
Copy Markdown
Member

Ah, so it is an explicit narrowing to itself. I don't know how common it is, it doesn't seem like something you want to be doing, but then again you might be programming defensively and you want to validate at run time that something has a type, so I can see a case for writing this kind of code.
I think I agree that, as long as it's cheap to check and doesn't make anything slower, there's not a lot of harm in doing that.

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.

Don't know if you're still waiting on my review for this, but this can be a reminder to merge this at some point~

@jakebailey

Copy link
Copy Markdown
MemberAuthor

I was, kinda; mainly I was trying to decide if this was even worthwhile if it's so rare and silly.

@jakebailey
Jake Bailey (jakebailey) merged commit 4b29ab5 into microsoft:mainNov 6, 2023
@jakebailey
Jake Bailey (jakebailey) deleted the fix-52345-other branch November 6, 2023 23:50
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 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

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants

@jakebailey@typescript-bot@gabritto@weswigham@sandersn