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.

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

Filter origin types when filtering union types #42378

Merged
merged 2 commits into from
Jan 20, 2021
Merged

Filter origin types when filtering union types #42378

merged 2 commits into from
Jan 20, 2021

Conversation

ahejlsberg
Copy link
Member

Fixes #42304.

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

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 17, 2021 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 17, 2021 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 17, 2021 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 17, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at db46250. 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 - master..42378

Metric master 42378 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 322,728k (± 0.01%) 322,620k (± 0.08%) -108k (- 0.03%) 321,645k 322,788k
Parse Time 2.00s (± 0.48%) 2.00s (± 0.34%) -0.01s (- 0.25%) 1.98s 2.01s
Bind Time 0.85s (± 1.04%) 0.85s (± 0.80%) +0.00s (+ 0.12%) 0.84s 0.87s
Check Time 4.89s (± 0.32%) 4.90s (± 0.47%) +0.01s (+ 0.16%) 4.85s 4.96s
Emit Time 5.57s (± 0.91%) 5.56s (± 0.82%) -0.00s (- 0.07%) 5.51s 5.74s
Total Time 13.31s (± 0.43%) 13.31s (± 0.48%) +0.00s (+ 0.02%) 13.25s 13.55s
Compiler-Unions - node (v14.15.1, x64)
Memory used 200,775k (± 0.50%) 200,607k (± 0.53%) -169k (- 0.08%) 199,620k 203,454k
Parse Time 0.81s (± 0.49%) 0.82s (± 0.46%) +0.01s (+ 0.74%) 0.81s 0.82s
Bind Time 0.53s (± 0.69%) 0.53s (± 0.90%) +0.00s (+ 0.19%) 0.52s 0.54s
Check Time 9.78s (± 0.44%) 9.82s (± 0.39%) +0.04s (+ 0.44%) 9.76s 9.90s
Emit Time 2.33s (± 1.50%) 2.35s (± 1.46%) +0.02s (+ 0.64%) 2.29s 2.46s
Total Time 13.44s (± 0.40%) 13.51s (± 0.44%) +0.06s (+ 0.47%) 13.43s 13.69s
Monaco - node (v14.15.1, x64)
Memory used 336,848k (± 0.01%) 336,817k (± 0.00%) -31k (- 0.01%) 336,792k 336,860k
Parse Time 1.65s (± 0.73%) 1.64s (± 0.54%) -0.01s (- 0.61%) 1.62s 1.66s
Bind Time 0.73s (± 0.85%) 0.73s (± 0.50%) +0.01s (+ 0.69%) 0.73s 0.74s
Check Time 4.86s (± 0.44%) 4.85s (± 0.44%) -0.01s (- 0.14%) 4.81s 4.91s
Emit Time 2.93s (± 0.72%) 2.93s (± 0.70%) -0.00s (- 0.14%) 2.90s 3.00s
Total Time 10.17s (± 0.41%) 10.16s (± 0.32%) -0.02s (- 0.16%) 10.07s 10.24s
TFS - node (v14.15.1, x64)
Memory used 291,586k (± 0.00%) 291,593k (± 0.01%) +7k (+ 0.00%) 291,555k 291,648k
Parse Time 1.29s (± 0.75%) 1.30s (± 1.37%) +0.01s (+ 0.85%) 1.28s 1.36s
Bind Time 0.68s (± 0.54%) 0.69s (± 0.68%) +0.00s (+ 0.73%) 0.68s 0.70s
Check Time 4.51s (± 0.62%) 4.50s (± 0.45%) -0.02s (- 0.33%) 4.44s 4.54s
Emit Time 3.06s (± 0.69%) 3.05s (± 0.57%) -0.01s (- 0.26%) 3.02s 3.08s
Total Time 9.55s (± 0.43%) 9.54s (± 0.31%) -0.00s (- 0.03%) 9.48s 9.61s
material-ui - node (v14.15.1, x64)
Memory used 472,234k (± 0.05%) 472,533k (± 0.06%) +300k (+ 0.06%) 471,601k 472,810k
Parse Time 2.15s (± 0.66%) 2.13s (± 0.61%) -0.02s (- 0.84%) 2.11s 2.16s
Bind Time 0.70s (± 0.86%) 0.69s (± 0.90%) -0.01s (- 0.86%) 0.68s 0.70s
Check Time 12.71s (± 0.81%) 12.67s (± 0.72%) -0.04s (- 0.33%) 12.52s 12.86s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.55s (± 0.64%) 15.49s (± 0.59%) -0.06s (- 0.42%) 15.34s 15.68s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-198-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)
Benchmark Name Iterations
Current 42378 10
Baseline master 10

@typescript-bot typescript-bot mentioned this pull request Jan 17, 2021
Open
@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@ahejlsberg
Copy link
Member Author

Tests and performance all look good.

DanielRosenwasser
DanielRosenwasser reviewed Jan 19, 2021
View reviewed changes
src/compiler/checker.ts
let newOrigin: Type | undefined;
if (origin && origin.flags & TypeFlags.Union) {
// If the origin type is a (denormalized) union type, filter its non-union constituents. If that ends
// up removing a smaller number of types than in the normalized constituent set (meaning some of the
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This says smaller, but below you're checking for whether or not you've reduced the same number of elements.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's either going to be smaller or the same. It could never be larger.

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I'm saying is that the code is written in an inverted way, so it wasn't as obvious what's happening. I just realized it was written this way to share the same return path though.

Copy link
Member

Choose a reason for hiding this comment

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

createOriginUnionOrIntersectionType is the thing you can't use if you end up removing a smaller number of types, right? If the length diff is the same, then you can, otherwise you have to fall back to getUnionTypeFromSortedList, which will ignore the origin information.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, github didn't load the previous 3 comments before I commented. I guess the point is that I think the comment is not confusing, even if the code itself is.

@ahejlsberg ahejlsberg merged commit 028f14c into master Jan 20, 2021
@ahejlsberg ahejlsberg deleted the fix42304 branch January 20, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser left review comments

@sandersn sandersn sandersn approved these changes

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Control flow narrowing sometimes de-aliases unions
4 participants
@ahejlsberg @typescript-bot @sandersn @DanielRosenwasser