NotificationsYou must be signed in to change notification settings
Fork13.2k
Star107k

Avoid expensive relationship checking in mapped type member resolution#36754

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ahejlsberg merged 2 commits intomasterfrommappedTypeMemberResolution

Feb 12, 2020

Merged

Avoid expensive relationship checking in mapped type member resolution#36754

ahejlsberg merged 2 commits intomasterfrommappedTypeMemberResolution

Feb 12, 2020

Conversation

Copy link

Member

ahejlsberg commentedFeb 12, 2020

This PR removes an expensiveisTypeAssignableTo check from mapped type member resolution and replaces it with a shallowermaybeTypeOfKind. The full relationship check can generate a lot of work exploring simplifications and constraints. Replacing it with the shallow check improves total compile time of the repro in#36564 by 8-10% and also resolves circularities I'm seeing in another PR.

Avoid expensive relationship checking in mapped type member resolution

868b7b9

Copy link

MemberAuthor

ahejlsberg commentedFeb 12, 2020

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

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at868b7b9. You can monitor the buildhere. It should now contribute to this PR's status checks.

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized community code test suite on this PR at868b7b9. You can monitor the buildhere. It should now contribute to this PR's status checks.

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the extended test suite on this PR at868b7b9. You can monitor the buildhere. It should now contribute to this PR's status checks.

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the perf test suite on this PR at868b7b9. You can monitor the buildhere. It should now contribute to this PR's status checks.

Update:The results are in!

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020

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

Here they are:

Comparison Report - master..36754

Metric	master	36754	Delta	Best	Worst
Angular - node (v10.16.3, x64)
Memory used	359,212k (± 0.02%)	358,539k (± 0.02%)	-673k (- 0.19%)	358,360k	358,707k
Parse Time	1.63s (± 0.36%)	1.62s (± 0.42%)	-0.01s (- 0.68%)	1.60s	1.63s
Bind Time	0.88s (± 1.07%)	0.88s (± 0.94%)	+0.00s (+ 0.11%)	0.87s	0.90s
Check Time	4.65s (± 0.42%)	4.66s (± 0.56%)	+0.01s (+ 0.15%)	4.59s	4.72s
Emit Time	5.20s (± 0.66%)	5.19s (± 0.53%)	-0.01s (- 0.19%)	5.12s	5.24s
Total Time	12.36s (± 0.34%)	12.35s (± 0.40%)	-0.01s (- 0.10%)	12.21s	12.46s
Monaco - node (v10.16.3, x64)
Memory used	364,651k (± 0.01%)	364,715k (± 0.01%)	+64k (+ 0.02%)	364,651k	364,770k
Parse Time	1.25s (± 0.71%)	1.25s (± 0.71%)	-0.01s (- 0.56%)	1.23s	1.27s
Bind Time	0.78s (± 0.63%)	0.78s (± 0.48%)	-0.00s (- 0.13%)	0.77s	0.78s
Check Time	4.68s (± 0.38%)	4.67s (± 0.45%)	-0.01s (- 0.17%)	4.63s	4.71s
Emit Time	2.87s (± 0.74%)	2.88s (± 0.87%)	+0.01s (+ 0.38%)	2.79s	2.91s
Total Time	9.58s (± 0.34%)	9.57s (± 0.44%)	-0.01s (- 0.09%)	9.42s	9.63s
TFS - node (v10.16.3, x64)
Memory used	324,193k (± 0.03%)	324,187k (± 0.03%)	-6k (- 0.00%)	324,017k	324,419k
Parse Time	0.95s (± 0.55%)	0.95s (± 0.52%)	-0.01s (- 0.53%)	0.94s	0.96s
Bind Time	0.74s (± 1.28%)	0.74s (± 1.25%)	+0.01s (+ 0.68%)	0.72s	0.76s
Check Time	4.22s (± 0.55%)	4.22s (± 0.36%)	-0.00s (- 0.09%)	4.19s	4.26s
Emit Time	3.00s (± 1.02%)	2.98s (± 1.00%)	-0.01s (- 0.40%)	2.92s	3.04s
Total Time	8.90s (± 0.54%)	8.89s (± 0.24%)	-0.01s (- 0.15%)	8.84s	8.94s
Angular - node (v12.1.0, x64)
Memory used	334,863k (± 0.02%)	334,216k (± 0.07%)	-648k (- 0.19%)	333,374k	334,526k
Parse Time	1.58s (± 0.70%)	1.57s (± 0.43%)	-0.01s (- 0.89%)	1.55s	1.58s
Bind Time	0.87s (± 0.81%)	0.87s (± 0.66%)	+0.00s (+ 0.46%)	0.86s	0.89s
Check Time	4.57s (± 0.51%)	4.57s (± 0.39%)	-0.00s (- 0.09%)	4.54s	4.61s
Emit Time	5.36s (± 0.86%)	5.39s (± 1.14%)	+0.03s (+ 0.50%)	5.28s	5.55s
Total Time	12.39s (± 0.47%)	12.39s (± 0.59%)	+0.01s (+ 0.07%)	12.29s	12.60s
Monaco - node (v12.1.0, x64)
Memory used	344,581k (± 0.01%)	344,536k (± 0.01%)	-45k (- 0.01%)	344,416k	344,623k
Parse Time	1.22s (± 0.60%)	1.21s (± 0.62%)	-0.00s (- 0.33%)	1.19s	1.22s
Bind Time	0.75s (± 1.17%)	0.75s (± 0.74%)	-0.00s (- 0.27%)	0.73s	0.76s
Check Time	4.54s (± 0.45%)	4.54s (± 0.35%)	+0.00s (+ 0.02%)	4.50s	4.56s
Emit Time	2.95s (± 0.56%)	2.97s (± 1.14%)	+0.02s (+ 0.75%)	2.91s	3.07s
Total Time	9.45s (± 0.35%)	9.47s (± 0.55%)	+0.02s (+ 0.18%)	9.37s	9.59s
TFS - node (v12.1.0, x64)
Memory used	306,431k (± 0.02%)	306,397k (± 0.02%)	-35k (- 0.01%)	306,266k	306,604k
Parse Time	0.94s (± 0.61%)	0.93s (± 0.60%)	-0.01s (- 0.85%)	0.92s	0.95s
Bind Time	0.70s (± 1.17%)	0.71s (± 1.06%)	+0.00s (+ 0.43%)	0.70s	0.73s
Check Time	4.16s (± 0.46%)	4.18s (± 0.37%)	+0.02s (+ 0.48%)	4.15s	4.21s
Emit Time	3.05s (± 1.27%)	3.06s (± 0.51%)	+0.01s (+ 0.23%)	3.04s	3.11s
Total Time	8.86s (± 0.47%)	8.88s (± 0.30%)	+0.02s (+ 0.27%)	8.85s	8.95s
Angular - node (v8.9.0, x64)
Memory used	354,068k (± 0.01%)	353,483k (± 0.01%)	-586k (- 0.17%)	353,349k	353,621k
Parse Time	2.13s (± 0.69%)	2.12s (± 0.31%)	-0.01s (- 0.38%)	2.11s	2.14s
Bind Time	0.93s (± 1.08%)	0.93s (± 0.48%)	-0.00s (- 0.21%)	0.92s	0.94s
Check Time	5.42s (± 0.50%)	5.43s (± 0.46%)	+0.00s (+ 0.04%)	5.38s	5.49s
Emit Time	6.23s (± 0.66%)	6.21s (± 0.70%)	-0.02s (- 0.32%)	6.10s	6.30s
Total Time	14.72s (± 0.35%)	14.69s (± 0.43%)	-0.03s (- 0.20%)	14.55s	14.84s
Monaco - node (v8.9.0, x64)
Memory used	362,949k (± 0.01%)	362,987k (± 0.02%)	+37k (+ 0.01%)	362,864k	363,143k
Parse Time	1.56s (± 0.48%)	1.56s (± 0.37%)	-0.00s (- 0.26%)	1.55s	1.57s
Bind Time	0.95s (± 0.38%)	0.95s (± 0.35%)	+0.00s (+ 0.42%)	0.94s	0.96s
Check Time	5.39s (± 1.60%)	5.35s (± 1.35%)	-0.04s (- 0.67%)	5.22s	5.52s
Emit Time	3.31s (± 3.84%)	3.33s (± 2.83%)	+0.03s (+ 0.88%)	3.01s	3.47s
Total Time	11.21s (± 0.72%)	11.20s (± 0.50%)	-0.01s (- 0.04%)	11.04s	11.34s
TFS - node (v8.9.0, x64)
Memory used	323,510k (± 0.01%)	323,495k (± 0.01%)	-16k (- 0.00%)	323,395k	323,567k
Parse Time	1.26s (± 0.60%)	1.26s (± 0.61%)	-0.00s (- 0.32%)	1.25s	1.28s
Bind Time	0.75s (± 0.45%)	0.75s (± 0.48%)	+0.00s (+ 0.13%)	0.75s	0.76s
Check Time	4.84s (± 0.63%)	4.79s (± 0.43%)	-0.05s (- 1.14%)	4.75s	4.83s
Emit Time	3.20s (± 0.40%)	3.17s (± 0.47%)	-0.02s (- 0.75%)	3.15s	3.21s
Total Time	10.06s (± 0.33%)	9.98s (± 0.33%)	-0.08s (- 0.82%)	9.91s	10.05s
Angular - node (v8.9.0, x86)
Memory used	201,227k (± 0.03%)	200,934k (± 0.04%)	-293k (- 0.15%)	200,798k	201,079k
Parse Time	2.05s (± 0.50%)	2.05s (± 0.60%)	+0.00s (+ 0.05%)	2.03s	2.08s
Bind Time	1.06s (± 0.64%)	1.06s (± 0.57%)	-0.00s (- 0.09%)	1.04s	1.07s
Check Time	4.95s (± 0.64%)	4.93s (± 0.33%)	-0.02s (- 0.46%)	4.89s	4.97s
Emit Time	6.16s (± 1.92%)	6.16s (± 1.94%)	+0.00s (+ 0.00%)	5.98s	6.42s
Total Time	14.22s (± 0.72%)	14.19s (± 0.80%)	-0.03s (- 0.22%)	13.99s	14.41s
Monaco - node (v8.9.0, x86)
Memory used	203,815k (± 0.02%)	203,777k (± 0.01%)	-38k (- 0.02%)	203,736k	203,833k
Parse Time	1.60s (± 0.52%)	1.60s (± 0.53%)	-0.01s (- 0.31%)	1.58s	1.62s
Bind Time	0.77s (± 0.44%)	0.76s (± 0.65%)	-0.01s (- 1.03%)	0.76s	0.78s
Check Time	5.19s (± 1.86%)	5.13s (± 0.68%)	-0.06s (- 1.21%)	5.08s	5.23s
Emit Time	3.13s (± 2.62%)	3.17s (± 1.29%)	+0.04s (+ 1.18%)	3.07s	3.23s
Total Time	10.70s (± 0.27%)	10.66s (± 0.18%)	-0.04s (- 0.36%)	10.62s	10.71s
TFS - node (v8.9.0, x86)
Memory used	182,626k (± 0.02%)	182,637k (± 0.01%)	+12k (+ 0.01%)	182,591k	182,704k
Parse Time	1.31s (± 0.67%)	1.30s (± 0.58%)	-0.01s (- 0.77%)	1.28s	1.31s
Bind Time	0.71s (± 0.73%)	0.71s (± 0.81%)	-0.00s (- 0.14%)	0.70s	0.72s
Check Time	4.59s (± 0.84%)	4.56s (± 0.85%)	-0.02s (- 0.50%)	4.50s	4.69s
Emit Time	2.95s (± 0.89%)	2.97s (± 1.43%)	+0.02s (+ 0.81%)	2.90s	3.09s
Total Time	9.55s (± 0.35%)	9.54s (± 0.52%)	-0.01s (- 0.08%)	9.40s	9.66s

System

Machine Name	ts-ci-ubuntu
Platform	linux 4.4.0-166-generic
Architecture	x64
Available Memory	16 GB
Available Memory	1 GB
CPUs	4 × 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)

Benchmark	Name	Iterations
Current	36754	10
Baseline	master	10

typescript-bot mentioned this pull request

Feb 12, 2020

🤖 User test baselines have changed for mappedTypeMemberResolution#36755

Closed

Copy link

Collaborator

typescript-bot commentedFeb 12, 2020

The user suite test run you requested has finished andfailed. I've opened aPR with the baseline diff from master.

Accept new baselines

283118c

Copy link

MemberAuthor

ahejlsberg commentedFeb 12, 2020

RWC and DT tests are clean (errors are preexisting conditions in master). Only positive effects on performance. I think this one is good to go.

Copy link

MemberAuthor

ahejlsberg commentedFeb 12, 2020

@amcasey @RyanCavanaugh Can't add you as reviewers (the usual GitHub issue), but please tak a look when you can.

weswigham reviewed

Feb 12, 2020

View reviewed changes

tests/baselines/reference/definiteAssignmentOfDestructuredVariable.types

		>foo : { [P in keyof T]: T[P]; }

		!(a && b);
		>!(a && b) :false

Copy link

Member

weswighamFeb 12, 2020

Choose a reason for hiding this comment

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

so.... this was just...wrong before?a wasT["a"] whichshould have been constrained tonumber | object | undefined, which meansa can be falsey, which means the new result is correct.

Do we fail to includeundefined constraints in our analysis somewhere in control flow?

Copy link

MemberAuthor

ahejlsbergFeb 12, 2020

Choose a reason for hiding this comment

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

I believe it's the old issue of not having a good representation for the falsy parts of a higher-order type. For example:

interfaceOptions{a:unknown;b:unknown;}functionfoo<TextendsOptions>(a:T['a'],b:T['b']){constx=a&&b;// Type T['b']}

The type ofx really should beT['b'] unioned with the falsy parts ofT['a'], but even with negated types this would be a pretty ugly type. IIRC we previously decided to leave this as a known design limitation.

Copy link

Member

weswighamFeb 12, 2020

Choose a reason for hiding this comment

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

Aw, yeah :(

weswigham approved these changes

Feb 12, 2020

View reviewed changes

Copy link

Member

weswigham left a comment

Choose a reason for hiding this comment

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

My above remark is moreso that the baseline change maybe indicates a bug elsewhere in the compiler that this change now works around. This, however, seems fine. The "worst case" is that we mix in an extraundefined that's already expressed in the property type's constraint - that's a little uglier for printback, but isn'tsupposed to change semantic meaning much (if any).

Maybe an inline comment on why we're making this change on the appropriate line is relevant, so someone who might come back in the future, looking to cleanup the printback in that edge case, doesn't swap it back and regress perf?

Copy link

MemberAuthor

ahejlsberg commentedFeb 12, 2020•
edited
Loading

The "worst case" is that we mix in an extra undefined that's already expressed in the property type's constraint

Yup, that's exactly it.

Maybe an inline comment on why we're making this change on the appropriate line is relevant

Don't think we need a comment. With this change the code is actually more consistent with what we do on the next line to shallowly removeundefined from the type.

But, for this and other reasons, it definitely would make sense to add the repro from#36564 as a performance test case. That way we'll see a 10% regression if someone messes with it.