Skip to content
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.

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

Restore ordering of operations involving type parameters and unions #50116

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

ahejlsberg
Copy link
Member

Fixes #49982.

I didn't add a regression test as the repro is quite large and involves external NPM packages. I have however manually verified that performance of the repro is back to 4.5 level and that no OOM occurs.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 31, 2022
@ahejlsberg
Copy link
Member Author

@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 Jul 31, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 31, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 31, 2022

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at e0c96cd. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 31, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..50116

Metric main 50116 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 335,995k (± 0.01%) 336,001k (± 0.00%) +6k (+ 0.00%) 335,969k 336,032k
Parse Time 2.07s (± 0.81%) 2.06s (± 0.40%) -0.01s (- 0.39%) 2.05s 2.09s
Bind Time 0.89s (± 0.66%) 0.89s (± 0.55%) +0.00s (+ 0.00%) 0.88s 0.90s
Check Time 5.82s (± 0.48%) 5.82s (± 0.47%) +0.00s (+ 0.03%) 5.77s 5.90s
Emit Time 6.36s (± 0.72%) 6.40s (± 0.63%) +0.04s (+ 0.60%) 6.30s 6.48s
Total Time 15.14s (± 0.53%) 15.17s (± 0.42%) +0.03s (+ 0.18%) 15.05s 15.33s
Compiler-Unions - node (v14.15.1, x64)
Memory used 193,155k (± 0.02%) 191,746k (± 0.02%) -1,409k (- 0.73%) 191,647k 191,811k
Parse Time 0.85s (± 0.78%) 0.85s (± 1.03%) 0.00s ( 0.00%) 0.84s 0.88s
Bind Time 0.57s (± 0.91%) 0.57s (± 0.91%) -0.00s (- 0.00%) 0.56s 0.58s
Check Time 6.74s (± 0.47%) 6.68s (± 0.67%) -0.06s (- 0.83%) 6.60s 6.83s
Emit Time 2.50s (± 0.49%) 2.48s (± 0.97%) -0.02s (- 0.76%) 2.44s 2.54s
Total Time 10.66s (± 0.34%) 10.58s (± 0.45%) -0.08s (- 0.77%) 10.48s 10.70s
Monaco - node (v14.15.1, x64)
Memory used 325,669k (± 0.01%) 325,647k (± 0.01%) -21k (- 0.01%) 325,612k 325,703k
Parse Time 1.58s (± 0.46%) 1.57s (± 0.69%) -0.01s (- 0.63%) 1.56s 1.61s
Bind Time 0.79s (± 0.97%) 0.78s (± 0.67%) -0.01s (- 0.76%) 0.77s 0.79s
Check Time 5.68s (± 0.55%) 5.66s (± 0.52%) -0.02s (- 0.30%) 5.62s 5.75s
Emit Time 3.35s (± 0.94%) 3.37s (± 1.11%) +0.02s (+ 0.69%) 3.32s 3.48s
Total Time 11.40s (± 0.54%) 11.38s (± 0.42%) -0.02s (- 0.14%) 11.29s 11.53s
TFS - node (v14.15.1, x64)
Memory used 288,816k (± 0.01%) 288,823k (± 0.01%) +7k (+ 0.00%) 288,765k 288,877k
Parse Time 1.33s (± 1.34%) 1.31s (± 1.48%) -0.01s (- 1.05%) 1.29s 1.35s
Bind Time 0.75s (± 3.33%) 0.78s (± 5.47%) +0.03s (+ 3.98%) 0.73s 0.90s
Check Time 5.37s (± 0.58%) 5.35s (± 0.32%) -0.01s (- 0.26%) 5.31s 5.39s
Emit Time 3.55s (± 2.16%) 3.65s (± 1.57%) +0.10s (+ 2.93%) 3.44s 3.72s
Total Time 11.00s (± 0.87%) 11.10s (± 0.68%) +0.10s (+ 0.94%) 10.85s 11.22s
material-ui - node (v14.15.1, x64)
Memory used 446,727k (± 0.01%) 446,728k (± 0.01%) +1k (+ 0.00%) 446,686k 446,828k
Parse Time 1.89s (± 0.70%) 1.88s (± 0.63%) -0.01s (- 0.53%) 1.86s 1.92s
Bind Time 0.72s (± 1.03%) 0.72s (± 0.69%) +0.00s (+ 0.69%) 0.72s 0.74s
Check Time 13.18s (± 0.54%) 13.10s (± 0.71%) -0.08s (- 0.62%) 12.92s 13.36s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.79s (± 0.50%) 15.70s (± 0.59%) -0.09s (- 0.58%) 15.51s 15.96s
xstate - node (v14.15.1, x64)
Memory used 541,534k (± 0.00%) 541,482k (± 0.00%) -52k (- 0.01%) 541,458k 541,517k
Parse Time 2.61s (± 0.41%) 2.59s (± 0.33%) -0.02s (- 0.69%) 2.56s 2.60s
Bind Time 1.16s (± 0.96%) 1.15s (± 0.93%) -0.01s (- 0.61%) 1.12s 1.17s
Check Time 1.56s (± 0.53%) 1.54s (± 0.62%) -0.01s (- 0.77%) 1.52s 1.56s
Emit Time 0.07s (± 4.13%) 0.07s (± 4.13%) 0.00s ( 0.00%) 0.07s 0.08s
Total Time 5.39s (± 0.29%) 5.35s (± 0.22%) -0.03s (- 0.63%) 5.34s 5.39s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 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 50116 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

@typescript-bot
Copy link
Collaborator

Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@ahejlsberg
Copy link
Member Author

@RyanCavanaugh @amcasey Test and performance suites all look good. We may want to consider this for 4.8.

@amcasey
Copy link
Member

amcasey commented Aug 1, 2022

I don't object to including it in 4.8, but I'm not convinced it's urgent, given that we don't seem to have heard about this in 4.6 or 4.7. Was the repro just a canary for a more widespread problem?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

This is basically a straight revert, so LGTM

Copy link

@ProKashif ProKashif left a comment

Choose a reason for hiding this comment

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

LGTM

@ahejlsberg ahejlsberg merged commit 697935d into main Aug 3, 2022
@ahejlsberg ahejlsberg deleted the fix49982 branch August 3, 2022 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS 4.6.2+ JavaScript heap out of memory
5 participants