Skip to content

Fix caching of union constituent instantiations that share a declaration #41003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix caching of union constituent instantiations that share a declaration #41003

wants to merge 3 commits into from

Conversation

andrewbranch
Copy link
Member

Not ready for review.

Fixes #40995

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

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 8, 2020 •
edited
Loading

Heya @andrewbranch, I've started to run the perf test suite on this PR at 1398152. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..41003

Metric master 41003 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 349,757k (± 0.02%) 349,964k (± 0.02%) +207k (+ 0.06%) 349,751k 350,115k
Parse Time 2.01s (± 0.80%) 2.00s (± 0.38%) -0.01s (- 0.65%) 1.97s 2.01s
Bind Time 0.82s (± 1.07%) 0.83s (± 0.72%) +0.00s (+ 0.36%) 0.82s 0.84s
Check Time 4.94s (± 0.54%) 4.94s (± 0.44%) +0.00s (+ 0.06%) 4.90s 4.99s
Emit Time 5.19s (± 0.41%) 5.23s (± 1.15%) +0.04s (+ 0.83%) 5.15s 5.43s
Total Time 12.96s (± 0.34%) 12.99s (± 0.59%) +0.03s (+ 0.24%) 12.87s 13.21s
Monaco - node (v10.16.3, x64)
Memory used 354,351k (± 0.02%) 354,650k (± 0.01%) +299k (+ 0.08%) 354,553k 354,743k
Parse Time 1.57s (± 0.57%) 1.56s (± 0.51%) -0.01s (- 0.51%) 1.54s 1.58s
Bind Time 0.71s (± 0.56%) 0.72s (± 0.51%) +0.00s (+ 0.70%) 0.71s 0.72s
Check Time 5.07s (± 0.54%) 5.09s (± 0.46%) +0.02s (+ 0.30%) 5.04s 5.13s
Emit Time 2.76s (± 0.93%) 2.78s (± 0.82%) +0.01s (+ 0.47%) 2.74s 2.82s
Total Time 10.11s (± 0.37%) 10.14s (± 0.41%) +0.03s (+ 0.27%) 10.06s 10.25s
TFS - node (v10.16.3, x64)
Memory used 307,640k (± 0.04%) 308,046k (± 0.02%) +406k (+ 0.13%) 307,860k 308,206k
Parse Time 1.22s (± 0.76%) 1.22s (± 0.56%) -0.01s (- 0.57%) 1.20s 1.23s
Bind Time 0.66s (± 1.37%) 0.67s (± 1.09%) +0.01s (+ 1.06%) 0.65s 0.68s
Check Time 4.57s (± 0.94%) 4.58s (± 0.69%) +0.01s (+ 0.18%) 4.53s 4.68s
Emit Time 2.90s (± 1.72%) 2.90s (± 0.89%) +0.00s (+ 0.14%) 2.85s 2.97s
Total Time 9.35s (± 0.59%) 9.36s (± 0.40%) +0.01s (+ 0.15%) 9.29s 9.45s
material-ui - node (v10.16.3, x64)
Memory used 489,125k (± 0.02%) 593,256k (± 0.01%) +104,131k (+21.29%) 593,134k 593,393k
Parse Time 1.99s (± 0.54%) 1.98s (± 0.54%) -0.01s (- 0.60%) 1.96s 2.00s
Bind Time 0.65s (± 0.90%) 0.65s (± 0.52%) +0.01s (+ 0.93%) 0.65s 0.66s
Check Time 13.42s (± 0.51%) 14.82s (± 0.29%) +1.40s (+10.41%) 14.76s 14.94s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.06s (± 0.45%) 17.45s (± 0.29%) +1.39s (+ 8.67%) 17.37s 17.58s
Angular - node (v12.1.0, x64)
Memory used 326,934k (± 0.02%) 327,134k (± 0.02%) +200k (+ 0.06%) 327,045k 327,288k
Parse Time 2.00s (± 0.53%) 1.99s (± 0.54%) -0.01s (- 0.45%) 1.97s 2.01s
Bind Time 0.81s (± 1.12%) 0.81s (± 0.64%) +0.00s (+ 0.00%) 0.80s 0.82s
Check Time 4.83s (± 0.40%) 4.85s (± 0.39%) +0.02s (+ 0.35%) 4.81s 4.89s
Emit Time 5.39s (± 0.84%) 5.42s (± 1.10%) +0.03s (+ 0.54%) 5.34s 5.57s
Total Time 13.04s (± 0.52%) 13.08s (± 0.52%) +0.04s (+ 0.30%) 12.96s 13.24s
Monaco - node (v12.1.0, x64)
Memory used 336,578k (± 0.02%) 336,734k (± 0.02%) +156k (+ 0.05%) 336,566k 336,875k
Parse Time 1.54s (± 0.72%) 1.52s (± 0.74%) -0.02s (- 1.11%) 1.50s 1.55s
Bind Time 0.69s (± 0.58%) 0.69s (± 0.58%) +0.00s (+ 0.29%) 0.68s 0.70s
Check Time 4.88s (± 0.50%) 4.88s (± 0.43%) 0.00s ( 0.00%) 4.84s 4.93s
Emit Time 2.82s (± 0.91%) 2.81s (± 0.53%) -0.01s (- 0.25%) 2.79s 2.85s
Total Time 9.93s (± 0.39%) 9.91s (± 0.39%) -0.02s (- 0.22%) 9.84s 10.02s
TFS - node (v12.1.0, x64)
Memory used 291,933k (± 0.03%) 292,202k (± 0.02%) +269k (+ 0.09%) 292,108k 292,305k
Parse Time 1.23s (± 0.49%) 1.23s (± 0.82%) +0.01s (+ 0.73%) 1.20s 1.25s
Bind Time 0.65s (± 1.38%) 0.64s (± 0.96%) -0.01s (- 0.77%) 0.63s 0.66s
Check Time 4.47s (± 0.30%) 4.49s (± 0.62%) +0.02s (+ 0.49%) 4.44s 4.56s
Emit Time 2.92s (± 0.99%) 2.95s (± 0.71%) +0.03s (+ 1.06%) 2.90s 2.99s
Total Time 9.26s (± 0.36%) 9.31s (± 0.34%) +0.06s (+ 0.60%) 9.25s 9.39s
material-ui - node (v12.1.0, x64)
Memory used 467,034k (± 0.05%) 570,165k (± 0.01%) +103,131k (+22.08%) 570,047k 570,238k
Parse Time 2.01s (± 0.37%) 2.00s (± 0.65%) -0.01s (- 0.45%) 1.98s 2.04s
Bind Time 0.64s (± 1.05%) 0.64s (± 0.97%) +0.00s (+ 0.31%) 0.63s 0.65s
Check Time 12.05s (± 0.94%) 13.22s (± 0.41%) +1.18s (+ 9.77%) 13.10s 13.37s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.70s (± 0.81%) 15.87s (± 0.38%) +1.17s (+ 7.95%) 15.73s 16.03s
Angular - node (v8.9.0, x64)
Memory used 346,442k (± 0.01%) 346,717k (± 0.01%) +275k (+ 0.08%) 346,602k 346,809k
Parse Time 2.54s (± 0.51%) 2.55s (± 0.35%) +0.01s (+ 0.39%) 2.54s 2.58s
Bind Time 0.85s (± 0.43%) 0.86s (± 0.79%) +0.01s (+ 1.29%) 0.85s 0.88s
Check Time 5.56s (± 0.52%) 5.61s (± 0.47%) +0.05s (+ 0.99%) 5.54s 5.67s
Emit Time 6.15s (± 1.07%) 6.13s (± 1.53%) -0.02s (- 0.26%) 5.97s 6.35s
Total Time 15.11s (± 0.55%) 15.17s (± 0.66%) +0.06s (+ 0.38%) 15.02s 15.43s
Monaco - node (v8.9.0, x64)
Memory used 355,704k (± 0.02%) 356,010k (± 0.01%) +305k (+ 0.09%) 355,925k 356,093k
Parse Time 1.89s (± 0.77%) 1.88s (± 0.35%) -0.00s (- 0.16%) 1.87s 1.90s
Bind Time 0.89s (± 0.45%) 0.89s (± 0.41%) -0.01s (- 0.56%) 0.88s 0.89s
Check Time 5.62s (± 0.37%) 5.66s (± 0.49%) +0.03s (+ 0.57%) 5.60s 5.72s
Emit Time 3.29s (± 1.17%) 3.25s (± 0.40%) -0.04s (- 1.19%) 3.22s 3.28s
Total Time 11.69s (± 0.28%) 11.67s (± 0.35%) -0.02s (- 0.14%) 11.58s 11.75s
TFS - node (v8.9.0, x64)
Memory used 309,391k (± 0.02%) 309,736k (± 0.01%) +345k (+ 0.11%) 309,668k 309,850k
Parse Time 1.55s (± 0.64%) 1.55s (± 0.40%) -0.00s (- 0.26%) 1.54s 1.56s
Bind Time 0.68s (± 0.73%) 0.67s (± 0.83%) -0.00s (- 0.59%) 0.66s 0.69s
Check Time 5.31s (± 0.58%) 5.30s (± 0.33%) -0.01s (- 0.17%) 5.27s 5.35s
Emit Time 2.94s (± 0.34%) 2.94s (± 0.93%) -0.00s (- 0.10%) 2.85s 2.98s
Total Time 10.48s (± 0.34%) 10.46s (± 0.34%) -0.02s (- 0.21%) 10.36s 10.52s
material-ui - node (v8.9.0, x64)
Memory used 493,431k (± 0.01%) 597,689k (± 0.01%) +104,259k (+21.13%) 597,567k 597,895k
Parse Time 2.40s (± 0.46%) 2.40s (± 0.37%) -0.00s (- 0.12%) 2.38s 2.42s
Bind Time 0.81s (± 1.02%) 0.81s (± 1.53%) -0.00s (- 0.12%) 0.79s 0.84s
Check Time 17.95s (± 0.69%) 19.79s (± 0.75%) +1.83s (+10.21%) 19.41s 20.12s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 21.17s (± 0.56%) 23.00s (± 0.67%) +1.83s (+ 8.65%) 22.61s 23.36s
Angular - node (v8.9.0, x86)
Memory used 198,690k (± 0.01%) 198,843k (± 0.03%) +153k (+ 0.08%) 198,742k 199,005k
Parse Time 2.48s (± 1.02%) 2.47s (± 0.70%) -0.01s (- 0.24%) 2.42s 2.50s
Bind Time 1.00s (± 1.00%) 1.00s (± 0.65%) -0.00s (- 0.10%) 0.99s 1.02s
Check Time 5.05s (± 0.56%) 5.03s (± 0.33%) -0.02s (- 0.32%) 4.99s 5.07s
Emit Time 5.89s (± 0.83%) 5.95s (± 0.92%) +0.06s (+ 1.05%) 5.84s 6.08s
Total Time 14.42s (± 0.58%) 14.45s (± 0.41%) +0.03s (+ 0.24%) 14.32s 14.57s
Monaco - node (v8.9.0, x86)
Memory used 201,493k (± 0.01%) 201,663k (± 0.02%) +170k (+ 0.08%) 201,601k 201,725k
Parse Time 1.94s (± 1.05%) 1.92s (± 1.08%) -0.02s (- 1.08%) 1.89s 1.99s
Bind Time 0.71s (± 0.47%) 0.71s (± 1.70%) -0.00s (- 0.42%) 0.69s 0.74s
Check Time 5.47s (± 0.38%) 5.54s (± 1.47%) +0.08s (+ 1.43%) 5.41s 5.74s
Emit Time 3.06s (± 0.59%) 2.97s (± 4.92%) -0.08s (- 2.68%) 2.66s 3.16s
Total Time 11.18s (± 0.25%) 11.15s (± 0.95%) -0.03s (- 0.24%) 10.93s 11.38s
TFS - node (v8.9.0, x86)
Memory used 176,867k (± 0.03%) 177,066k (± 0.03%) +199k (+ 0.11%) 176,996k 177,231k
Parse Time 1.60s (± 1.10%) 1.60s (± 2.19%) -0.00s (- 0.31%) 1.56s 1.73s
Bind Time 0.66s (± 2.04%) 0.65s (± 1.23%) -0.01s (- 1.07%) 0.64s 0.67s
Check Time 4.82s (± 0.58%) 4.84s (± 0.62%) +0.02s (+ 0.37%) 4.80s 4.93s
Emit Time 2.80s (± 0.66%) 2.83s (± 1.25%) +0.03s (+ 0.93%) 2.76s 2.92s
Total Time 9.88s (± 0.51%) 9.92s (± 0.49%) +0.04s (+ 0.35%) 9.83s 10.09s
material-ui - node (v8.9.0, x86)
Memory used 277,878k (± 0.02%) 330,120k (± 0.01%) +52,243k (+18.80%) 330,051k 330,177k
Parse Time 2.46s (± 0.43%) 2.46s (± 0.58%) -0.00s (- 0.08%) 2.43s 2.49s
Bind Time 0.69s (± 1.35%) 0.69s (± 1.60%) +0.01s (+ 0.87%) 0.67s 0.72s
Check Time 16.45s (± 0.73%) 18.14s (± 0.69%) +1.69s (+10.24%) 17.68s 18.30s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.60s (± 0.63%) 21.28s (± 0.63%) +1.69s (+ 8.61%) 20.80s 21.45s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory3 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 41003 10
Baseline master 10

@andrewbranch
Copy link
Member Author

Of course, a huge increase for material-ui, <0.5% difference everywhere else 🤦

@andrewbranch
Copy link
Member Author

@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 12, 2020 •
edited
Loading

Heya @andrewbranch, I've started to run the perf test suite on this PR at 2d9829d. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 12, 2020 •
edited
Loading

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 2d9829d. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 12, 2020 •
edited
Loading

Hey @andrewbranch, 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/87462/artifacts?artifactName=tgz&fileId=BFC3758578CD4E1D63B48DA92E00B29A45C39527D8E4198B95066582761D3F6302&fileName=/typescript-4.1.0-insiders.20201012.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..41003

Metric master 41003 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 349,757k (± 0.02%) 350,182k (± 0.02%) +425k (+ 0.12%) 350,092k 350,349k
Parse Time 2.01s (± 0.80%) 2.00s (± 0.77%) -0.01s (- 0.40%) 1.98s 2.05s
Bind Time 0.82s (± 1.07%) 0.83s (± 1.18%) +0.01s (+ 1.09%) 0.82s 0.87s
Check Time 4.94s (± 0.54%) 4.97s (± 0.95%) +0.03s (+ 0.55%) 4.87s 5.07s
Emit Time 5.19s (± 0.41%) 5.24s (± 0.52%) +0.05s (+ 0.96%) 5.18s 5.31s
Total Time 12.96s (± 0.34%) 13.04s (± 0.54%) +0.07s (+ 0.57%) 12.89s 13.19s
Monaco - node (v10.16.3, x64)
Memory used 354,351k (± 0.02%) 354,362k (± 0.02%) +11k (+ 0.00%) 354,130k 354,539k
Parse Time 1.57s (± 0.57%) 1.56s (± 0.53%) -0.01s (- 0.70%) 1.54s 1.57s
Bind Time 0.71s (± 0.56%) 0.71s (± 0.70%) +0.00s (+ 0.56%) 0.71s 0.73s
Check Time 5.07s (± 0.54%) 5.08s (± 0.44%) +0.00s (+ 0.10%) 5.03s 5.13s
Emit Time 2.76s (± 0.93%) 2.77s (± 1.10%) +0.01s (+ 0.22%) 2.73s 2.88s
Total Time 10.11s (± 0.37%) 10.12s (± 0.49%) +0.00s (+ 0.04%) 10.06s 10.30s
TFS - node (v10.16.3, x64)
Memory used 307,640k (± 0.04%) 307,676k (± 0.01%) +35k (+ 0.01%) 307,568k 307,766k
Parse Time 1.22s (± 0.76%) 1.21s (± 0.56%) -0.01s (- 0.57%) 1.21s 1.24s
Bind Time 0.66s (± 1.37%) 0.67s (± 1.04%) +0.01s (+ 1.51%) 0.65s 0.68s
Check Time 4.57s (± 0.94%) 4.58s (± 0.68%) +0.02s (+ 0.37%) 4.52s 4.66s
Emit Time 2.90s (± 1.72%) 2.93s (± 1.33%) +0.03s (+ 0.93%) 2.80s 2.99s
Total Time 9.35s (± 0.59%) 9.40s (± 0.49%) +0.05s (+ 0.51%) 9.33s 9.52s
material-ui - node (v10.16.3, x64)
Memory used 489,125k (± 0.02%) 489,058k (± 0.01%) -67k (- 0.01%) 488,911k 489,148k
Parse Time 1.99s (± 0.54%) 1.98s (± 0.30%) -0.02s (- 0.80%) 1.96s 1.99s
Bind Time 0.65s (± 0.90%) 0.65s (± 1.23%) +0.00s (+ 0.15%) 0.63s 0.66s
Check Time 13.42s (± 0.51%) 13.39s (± 0.72%) -0.03s (- 0.22%) 13.23s 13.71s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.06s (± 0.45%) 16.02s (± 0.59%) -0.05s (- 0.29%) 15.87s 16.33s
Angular - node (v12.1.0, x64)
Memory used 326,934k (± 0.02%) 327,389k (± 0.01%) +455k (+ 0.14%) 327,298k 327,502k
Parse Time 2.00s (± 0.53%) 2.00s (± 0.55%) +0.00s (+ 0.15%) 1.99s 2.03s
Bind Time 0.81s (± 1.12%) 0.81s (± 0.72%) -0.00s (- 0.25%) 0.80s 0.82s
Check Time 4.83s (± 0.40%) 4.86s (± 0.45%) +0.02s (+ 0.52%) 4.83s 4.94s
Emit Time 5.39s (± 0.84%) 5.40s (± 0.60%) +0.01s (+ 0.11%) 5.36s 5.49s
Total Time 13.04s (± 0.52%) 13.07s (± 0.47%) +0.04s (+ 0.28%) 13.00s 13.29s
Monaco - node (v12.1.0, x64)
Memory used 336,578k (± 0.02%) 336,550k (± 0.02%) -28k (- 0.01%) 336,378k 336,677k
Parse Time 1.54s (± 0.72%) 1.54s (± 0.87%) +0.00s (+ 0.33%) 1.52s 1.58s
Bind Time 0.69s (± 0.58%) 0.69s (± 0.71%) +0.00s (+ 0.73%) 0.69s 0.71s
Check Time 4.88s (± 0.50%) 4.88s (± 0.31%) -0.00s (- 0.02%) 4.86s 4.91s
Emit Time 2.82s (± 0.91%) 2.79s (± 0.63%) -0.03s (- 1.06%) 2.77s 2.83s
Total Time 9.93s (± 0.39%) 9.91s (± 0.23%) -0.02s (- 0.21%) 9.84s 9.95s
TFS - node (v12.1.0, x64)
Memory used 291,933k (± 0.03%) 291,936k (± 0.02%) +2k (+ 0.00%) 291,831k 292,027k
Parse Time 1.23s (± 0.49%) 1.23s (± 0.72%) +0.01s (+ 0.65%) 1.22s 1.26s
Bind Time 0.65s (± 1.38%) 0.65s (± 0.92%) 0.00s ( 0.00%) 0.63s 0.66s
Check Time 4.47s (± 0.30%) 4.51s (± 0.32%) +0.05s (+ 1.10%) 4.49s 4.55s
Emit Time 2.92s (± 0.99%) 2.94s (± 0.98%) +0.02s (+ 0.72%) 2.87s 3.00s
Total Time 9.26s (± 0.36%) 9.33s (± 0.38%) +0.08s (+ 0.82%) 9.25s 9.42s
material-ui - node (v12.1.0, x64)
Memory used 467,034k (± 0.05%) 467,011k (± 0.05%) -23k (- 0.00%) 466,083k 467,195k
Parse Time 2.01s (± 0.37%) 2.00s (± 0.32%) -0.01s (- 0.55%) 1.99s 2.02s
Bind Time 0.64s (± 1.05%) 0.64s (± 1.21%) +0.00s (+ 0.31%) 0.63s 0.66s
Check Time 12.05s (± 0.94%) 12.04s (± 0.71%) -0.01s (- 0.08%) 11.94s 12.37s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.70s (± 0.81%) 14.68s (± 0.56%) -0.02s (- 0.14%) 14.59s 15.00s
Angular - node (v8.9.0, x64)
Memory used 346,442k (± 0.01%) 346,842k (± 0.02%) +400k (+ 0.12%) 346,654k 346,954k
Parse Time 2.54s (± 0.51%) 2.55s (± 0.53%) +0.00s (+ 0.16%) 2.53s 2.59s
Bind Time 0.85s (± 0.43%) 0.87s (± 0.42%) +0.01s (+ 1.41%) 0.86s 0.87s
Check Time 5.56s (± 0.52%) 5.59s (± 0.59%) +0.03s (+ 0.58%) 5.51s 5.65s
Emit Time 6.15s (± 1.07%) 6.23s (± 0.86%) +0.08s (+ 1.28%) 6.13s 6.38s
Total Time 15.11s (± 0.55%) 15.23s (± 0.44%) +0.13s (+ 0.83%) 15.10s 15.40s
Monaco - node (v8.9.0, x64)
Memory used 355,704k (± 0.02%) 355,783k (± 0.01%) +78k (+ 0.02%) 355,713k 355,862k
Parse Time 1.89s (± 0.77%) 1.89s (± 0.59%) 0.00s ( 0.00%) 1.86s 1.92s
Bind Time 0.89s (± 0.45%) 0.89s (± 0.56%) -0.01s (- 0.67%) 0.88s 0.90s
Check Time 5.62s (± 0.37%) 5.62s (± 0.31%) -0.01s (- 0.11%) 5.58s 5.65s
Emit Time 3.29s (± 1.17%) 3.25s (± 0.72%) -0.04s (- 1.13%) 3.20s 3.32s
Total Time 11.69s (± 0.28%) 11.64s (± 0.22%) -0.05s (- 0.43%) 11.59s 11.72s
TFS - node (v8.9.0, x64)
Memory used 309,391k (± 0.02%) 309,413k (± 0.02%) +22k (+ 0.01%) 309,338k 309,537k
Parse Time 1.55s (± 0.64%) 1.55s (± 0.40%) -0.00s (- 0.13%) 1.54s 1.57s
Bind Time 0.68s (± 0.73%) 0.67s (± 0.88%) -0.00s (- 0.30%) 0.66s 0.68s
Check Time 5.31s (± 0.58%) 5.30s (± 0.32%) -0.01s (- 0.19%) 5.27s 5.33s
Emit Time 2.94s (± 0.34%) 2.95s (± 0.64%) +0.01s (+ 0.48%) 2.91s 3.00s
Total Time 10.48s (± 0.34%) 10.48s (± 0.30%) -0.00s (- 0.01%) 10.41s 10.55s
material-ui - node (v8.9.0, x64)
Memory used 493,431k (± 0.01%) 493,481k (± 0.01%) +50k (+ 0.01%) 493,369k 493,591k
Parse Time 2.40s (± 0.46%) 2.40s (± 0.39%) -0.01s (- 0.21%) 2.38s 2.43s
Bind Time 0.81s (± 1.02%) 0.82s (± 0.91%) +0.01s (+ 0.74%) 0.81s 0.84s
Check Time 17.95s (± 0.69%) 17.98s (± 0.87%) +0.03s (+ 0.17%) 17.45s 18.24s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 21.17s (± 0.56%) 21.20s (± 0.74%) +0.03s (+ 0.14%) 20.67s 21.47s
Angular - node (v8.9.0, x86)
Memory used 198,690k (± 0.01%) 198,951k (± 0.02%) +261k (+ 0.13%) 198,874k 199,052k
Parse Time 2.48s (± 1.02%) 2.48s (± 0.79%) +0.01s (+ 0.24%) 2.44s 2.53s
Bind Time 1.00s (± 1.00%) 1.01s (± 0.99%) +0.00s (+ 0.40%) 0.99s 1.03s
Check Time 5.05s (± 0.56%) 5.06s (± 0.60%) +0.01s (+ 0.20%) 5.00s 5.13s
Emit Time 5.89s (± 0.83%) 5.98s (± 0.93%) +0.09s (+ 1.53%) 5.89s 6.14s
Total Time 14.42s (± 0.58%) 14.52s (± 0.53%) +0.11s (+ 0.74%) 14.36s 14.72s
Monaco - node (v8.9.0, x86)
Memory used 201,493k (± 0.01%) 201,510k (± 0.01%) +17k (+ 0.01%) 201,460k 201,576k
Parse Time 1.94s (± 1.05%) 1.92s (± 0.77%) -0.02s (- 1.18%) 1.89s 1.95s
Bind Time 0.71s (± 0.47%) 0.71s (± 0.66%) 0.00s ( 0.00%) 0.70s 0.72s
Check Time 5.47s (± 0.38%) 5.51s (± 1.29%) +0.04s (+ 0.75%) 5.39s 5.71s
Emit Time 3.06s (± 0.59%) 3.02s (± 3.49%) -0.03s (- 1.05%) 2.74s 3.15s
Total Time 11.18s (± 0.25%) 11.16s (± 0.60%) -0.02s (- 0.15%) 10.99s 11.31s
TFS - node (v8.9.0, x86)
Memory used 176,867k (± 0.03%) 176,971k (± 0.03%) +104k (+ 0.06%) 176,868k 177,081k
Parse Time 1.60s (± 1.10%) 1.60s (± 1.20%) -0.01s (- 0.37%) 1.56s 1.64s
Bind Time 0.66s (± 2.04%) 0.64s (± 0.76%) -0.01s (- 1.68%) 0.64s 0.66s
Check Time 4.82s (± 0.58%) 4.84s (± 0.52%) +0.02s (+ 0.35%) 4.78s 4.90s
Emit Time 2.80s (± 0.66%) 2.79s (± 1.00%) -0.01s (- 0.36%) 2.72s 2.84s
Total Time 9.88s (± 0.51%) 9.87s (± 0.54%) -0.01s (- 0.11%) 9.77s 9.98s
material-ui - node (v8.9.0, x86)
Memory used 277,878k (± 0.02%) 277,867k (± 0.02%) -11k (- 0.00%) 277,742k 277,967k
Parse Time 2.46s (± 0.43%) 2.46s (± 0.74%) +0.00s (+ 0.12%) 2.42s 2.52s
Bind Time 0.69s (± 1.35%) 0.71s (± 3.53%) +0.02s (+ 3.20%) 0.67s 0.79s
Check Time 16.45s (± 0.73%) 16.42s (± 0.46%) -0.03s (- 0.16%) 16.26s 16.61s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.60s (± 0.63%) 19.59s (± 0.38%) -0.00s (- 0.01%) 19.46s 19.76s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory3 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 41003 10
Baseline master 10

@andrewbranch
Copy link
Member Author

This is now strictly better than the current behavior, and has identical caching/performance characteristics, but I’m unsure whether it’s a complete fix. (That is, there may be more cases where returning a cached instantiation is wrong.)

@ahejlsberg
Copy link
Member

@andrewbranch It's not immediately obvious to me what's going on here. Would you mind explaining what the core issue is?

@andrewbranch
Copy link
Member Author

I’ll describe what happens before this change. This code errors because “No overload matches this call” on the .filter(...):

interface Success {
    isSuccess: true;
}

interface Fail {
    isSuccess: false;
}

function bar<Q>(a: (Success | Fail)[]) {
    a
        .map(item => ({...item}))
        .filter(value => {
            return value.isSuccess;
        });
}

The error elaboration for the overload that ought to match is

Overload 2 of 2, '(predicate: (value: { isSuccess: true; } | { isSuccess: false; }, index: number, array: ({ isSuccess: true; } | { isSuccess: false; })[]) => unknown, thisArg?: any): ({ isSuccess: true; } | { isSuccess: false; })[]', gave the following error.
    Argument of type '(value: { isSuccess: true; }) => true' is not assignable to parameter of type '(value: { isSuccess: true; } | { isSuccess: false; }, index: number, array: ({ isSuccess: true; } | { isSuccess: false; })[]) => unknown'.
      Types of parameters 'value' and 'value' are incompatible.
        Type '{ isSuccess: true; } | { isSuccess: false; }' is not assignable to type '{ isSuccess: true; }'.
          Type '{ isSuccess: false; }' is not assignable to type '{ isSuccess: true; }'.
            Types of property 'isSuccess' are incompatible.
              Type 'false' is not assignable to type 'true'.

and quick info on value shows { isSuccess: true } instead of the union, providing a clue as to what’s going wrong.

The first important thing that happens is the spread in the .map(...) call creates new type identities that are structurally identical to Success and Fail and Success | Fail (which is why printback no longer uses those names afterward), and both the new { isSuccess: true } and the new { isSuccess: false } types share their declaration site at { ...item }.

Where the problem arises is during signature resolution for .filter(...). We try on the first overload, which is

interface Array<T> {
  // ...
  filter<S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[];
  // ...
}

and try to infer S. Since our predicate is not a type predicate, inference initially produces unknown, which we will need to replace with the constraint of T, which is { isSuccess: true } | { isSuccess: false }. We first instantiate this constraint with the inference context’s non-fixing mapper, which ought to act as the identity function since this type does not reference S, but it goes off the rails.

We instantiate each union constituent through getObjectTypeInstantiation (we’ve finally made it to the code this PR changes). Recall that each constituent’s symbol’s declaration is { ...item }. We have an outer type parameter Q, which should have no effect on the instantiation in the end, but we don’t determine that without trying it. But first, we cache our union constituent { isSuccess: true } as the result of the instantiation for Q on the node links for { ...item } (this is the line I removed). When we move on to the second union constituent, { isSuccess: false }, we have a problem: since the declaration is the same, we already have links.outerTypeParameters cached, so we move on to the real work of the function (if (typeParameters.length) { ... }). We get the typeArguments for this instantiation by running the outer type parameters (Q) through the inference context’s mapper. Because Q was not the inference target, it returns Q, so our type arguments are just Q. We look up the instantiation for Q on the links, and get the result from the previous union constituent, { isSuccess: true }—clearly wrong.

So in short, the union { isSuccess: true } | { isSuccess: false } collapses to the first constituent during an instantiation that should have been a no-op because the first one gets cached as the instantiation of an unused outer type parameter on their shared declaration.

@ahejlsberg
Copy link
Member

Hmm, that's pretty subtle. The issue is that the logic in getObjectTypeInstantiation implicitly assumes only a single type is associated with the declaration. That was always the case when the logic was written, but with union type distribution over spreads in object literals it isn't anymore. The fix is to move the instantiation cache from the NodeLinks associated with the declaration to the target object type being instantiated. That'll allow multiple types to share the same declaration, but we'll still have a single shared cache for each type. I tried it out and it seems to work great. I'll put up a PR that you can poach from.

@ahejlsberg
Copy link
Member

#41084 is what I think is the correct fix.

@MartinJohns MartinJohns mentioned this pull request Oct 13, 2020
Closed
@andrewbranch
Copy link
Member Author

The fix is to move the instantiation cache from the NodeLinks associated with the declaration to the target object type being instantiated.

Ah, the first thing I tried was effectively equivalent to moving the cache to the type, which in my test case was the same as target, but defeated the purpose of the cache when an actual generic type had many instantiations (giving us the bad perf on material-ui). This makes more sense. Thanks!

@andrewbranch andrewbranch mentioned this pull request Oct 13, 2020
Merged
@andrewbranch andrewbranch deleted the bug/40995 branch October 14, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@andrewbranch andrewbranch

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.

typescript generic break filter after spreading object in map

3 participants

@andrewbranch @typescript-bot @ahejlsberg