Skip to content

Always cache relations involving intersection types#46523

Merged
ahejlsberg merged 3 commits intomainfrom
fix46500-2
Oct 26, 2021
Merged

Always cache relations involving intersection types#46523
ahejlsberg merged 3 commits intomainfrom
fix46500-2

Conversation

@ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Oct 25, 2021

An alternative fix for #46500, hopefully with less impact on the material-ui performance test.

Fixes #46500.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 25, 2021
@ahejlsberg
Copy link
Member Author

ahejlsberg commented Oct 25, 2021

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 8c278bf. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 8c278bf. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the inline community code test suite on this PR at 8c278bf. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 8c278bf. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..46523

Metric main 46523 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 330,354k (± 0.01%) 330,365k (± 0.01%) +10k (+ 0.00%) 330,337k 330,406k
Parse Time 1.95s (± 0.64%) 1.95s (± 0.25%) +0.00s (+ 0.00%) 1.94s 1.96s
Bind Time 0.86s (± 0.69%) 0.86s (± 1.00%) +0.01s (+ 0.70%) 0.85s 0.89s
Check Time 5.33s (± 0.43%) 5.35s (± 0.25%) +0.02s (+ 0.38%) 5.31s 5.37s
Emit Time 6.13s (± 0.50%) 6.12s (± 0.56%) -0.01s (- 0.18%) 6.04s 6.19s
Total Time 14.26s (± 0.26%) 14.27s (± 0.31%) +0.02s (+ 0.11%) 14.19s 14.34s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,567k (± 0.49%) 191,806k (± 0.69%) -761k (- 0.40%) 189,076k 193,274k
Parse Time 0.81s (± 0.69%) 0.81s (± 0.82%) +0.00s (+ 0.25%) 0.80s 0.83s
Bind Time 0.55s (± 0.40%) 0.55s (± 0.66%) +0.00s (+ 0.54%) 0.55s 0.56s
Check Time 7.53s (± 0.44%) 7.54s (± 0.41%) +0.01s (+ 0.13%) 7.48s 7.63s
Emit Time 2.44s (± 0.71%) 2.44s (± 1.00%) -0.01s (- 0.29%) 2.38s 2.48s
Total Time 11.34s (± 0.42%) 11.34s (± 0.45%) +0.01s (+ 0.04%) 11.20s 11.46s
Monaco - node (v14.15.1, x64)
Memory used 324,010k (± 0.01%) 323,995k (± 0.01%) -15k (- 0.00%) 323,947k 324,032k
Parse Time 1.51s (± 0.62%) 1.50s (± 0.33%) -0.00s (- 0.20%) 1.49s 1.51s
Bind Time 0.75s (± 0.48%) 0.75s (± 0.53%) -0.01s (- 0.66%) 0.74s 0.76s
Check Time 5.30s (± 0.42%) 5.29s (± 0.18%) -0.01s (- 0.11%) 5.27s 5.32s
Emit Time 3.20s (± 0.53%) 3.21s (± 0.34%) +0.01s (+ 0.41%) 3.19s 3.24s
Total Time 10.76s (± 0.31%) 10.76s (± 0.13%) -0.00s (- 0.04%) 10.73s 10.80s
TFS - node (v14.15.1, x64)
Memory used 288,365k (± 0.01%) 288,366k (± 0.01%) +1k (+ 0.00%) 288,324k 288,404k
Parse Time 1.23s (± 0.70%) 1.22s (± 0.67%) -0.01s (- 0.73%) 1.21s 1.25s
Bind Time 0.73s (± 0.76%) 0.73s (± 0.45%) -0.00s (- 0.27%) 0.72s 0.74s
Check Time 4.90s (± 0.53%) 4.90s (± 0.50%) +0.01s (+ 0.12%) 4.85s 4.95s
Emit Time 3.48s (± 0.69%) 3.48s (± 0.68%) +0.00s (+ 0.12%) 3.44s 3.54s
Total Time 10.34s (± 0.35%) 10.34s (± 0.46%) +0.00s (+ 0.01%) 10.25s 10.43s
material-ui - node (v14.15.1, x64)
Memory used 447,290k (± 0.08%) 448,412k (± 0.06%) +1,122k (+ 0.25%) 447,375k 448,582k
Parse Time 1.82s (± 0.49%) 1.83s (± 0.69%) +0.00s (+ 0.05%) 1.80s 1.86s
Bind Time 0.68s (± 0.54%) 0.68s (± 0.54%) 0.00s ( 0.00%) 0.67s 0.68s
Check Time 13.09s (± 0.97%) 13.07s (± 0.48%) -0.03s (- 0.19%) 12.93s 13.26s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.59s (± 0.84%) 15.57s (± 0.39%) -0.02s (- 0.15%) 15.42s 15.76s
xstate - node (v14.15.1, x64)
Memory used 533,836k (± 0.01%) 534,053k (± 0.01%) +217k (+ 0.04%) 534,016k 534,150k
Parse Time 2.55s (± 0.54%) 2.56s (± 0.52%) +0.01s (+ 0.39%) 2.54s 2.59s
Bind Time 1.15s (± 0.82%) 1.16s (± 1.25%) +0.01s (+ 0.61%) 1.13s 1.20s
Check Time 1.55s (± 0.54%) 1.54s (± 0.32%) -0.00s (- 0.00%) 1.54s 1.56s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.32s (± 0.36%) 5.33s (± 0.43%) +0.01s (+ 0.21%) 5.28s 5.40s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory10 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46523 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021

@ahejlsberg
Great news! no new errors were found between main..refs/pull/46523/merge

@ahejlsberg ahejlsberg mentioned this pull request Oct 25, 2021
Closed
@ahejlsberg
Copy link
Member Author

ahejlsberg commented Oct 25, 2021

No effect on performance from this one, so definitely a better fix than #46505.

andrewbranch
andrewbranch approved these changes Oct 25, 2021
View reviewed changes
tests/baselines/reference/unionTypeCallSignatures6.errors.txt
!!! error TS2769: Type 'void' is not assignable to type 'A'.
!!! error TS2769: Overload 2 of 2, '(this: A & B): void', gave the following error.
!!! error TS2769: The 'this' context of type 'void' is not assignable to method's 'this' of type 'A & B'.
!!! error TS2769: Type 'void' is not assignable to type 'A'.
Copy link
Member

@andrewbranch andrewbranch Oct 25, 2021

Choose a reason for hiding this comment

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

The loss of this ending of the elaboration is a little unfortunate since the line above it is kind of an earfull.

Copy link
Member Author

@ahejlsberg ahejlsberg Oct 26, 2021

Choose a reason for hiding this comment

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

Yeah, but that's basically the effect we get from caching the relation.

weswigham
weswigham approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Huh, this actually reduced memory usage for Compiler - Unions. We sure we can't also just cache all union relations as well, and not just those involving more than 4 union members? It'd simplify the code a bit.

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Oct 26, 2021

@weswigham We already tried that back when, but let's get some fresh data: #46528.

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Oct 26, 2021

Well, #46528 doesn't indicate much of a difference, but I'm going to be conservative and keep the non-caching behavior for small unions.

@ahejlsberg ahejlsberg merged commit 44c63a7 into main Oct 26, 2021
@ahejlsberg ahejlsberg deleted the fix46500-2 branch October 26, 2021 16:36
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 8, 2021

Was this meant to be picked into 4.5, or was it intentionally 4.6? The issue's milestone is 4.5.1.

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Nov 8, 2021

I think we should pick it for 4.5 if possible. It's a 4.5 specific regression, probably not too common, but hard to know. I'd say the fix is very safe indeed.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 8, 2021

@typescript-bot cherry-pick this to release-4.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 8, 2021 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.5 on this PR at 8c278bf. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 8, 2021

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.5 manually.

mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@ahejlsberg @mprobst
Always cache relations involving intersection types (microsoft#46523)
9c4d9da 
* Always cache relations involving intersection types

* Accept new baselines

* Add regression test
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham weswigham weswigham approved these changes

@andrewbranch andrewbranch andrewbranch approved these changes

Assignees

@ahejlsberg ahejlsberg

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TS 4.5 type instantiation is excessively deep and possibly infinite, with seemingly legal type

5 participants

@ahejlsberg @typescript-bot @DanielRosenwasser @weswigham @andrewbranch