Skip to content

do not make inferences with the same source\target pair multiple times #7163

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

Merged
merged 3 commits into from
Feb 22, 2016

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Feb 20, 2016

partial fix for #7081

var a = new Rx.Subject<string>();
var b = new Rx.Subject<Immutable.Set<number>>();
var c1 = a.withLatestFrom(b, x => x); // 1

Total compilation time for an original repro case (above) has dropped from +20 minutes to 1.5s (check time: ~0.9s ). However caching is local to every inference session so it still scales pretty badly

Number of times line (1) appear in the code Check time (s)
1 0.93
5 1.12
10 1.37
20 1.79
40 2.75
80 4.54
160 8.28
320 15.59

Note: I'm not sure how to write a self-contained test for this issue without including rx.all.d.ts and immutable.d.ts

@vladima
Copy link
Contributor Author

vladima commented Feb 20, 2016

as a subsequent step we can set the maximum depth of nesting when we explore the structure of types during type inference. This approach seems to be promising and it scales better - these are numbers for the same test with max depth = 5

Number of times line (1) appear in the code Check time (s)
1 0.81
5 0.88
10 0.93
20 1.08
40 1.27
80 1.72
160 2.69
320 4.49

However before moving forward I'd like to verify that setting max depth does not lead to serious regressions in type inference in our real world code test suite.

@DanielRosenwasser
Copy link
Member

LGTM, others should take a look

@mhegazy
Copy link
Contributor

mhegazy commented Feb 22, 2016

👍

@sandersn
Copy link
Member

Does this address the checking of typeof C vs instantiated typeof C in a generic static function? If so, you can use the test from #7138, at 7a48fb2.

@vladima
Copy link
Contributor Author

vladima commented Feb 22, 2016

no, #7097 is not fixed with this change

@sandersn
Copy link
Member

Oh well. :( 👍, but with the caveat that my grasp on the caching mechanism is not too strong.

vladima added a commit that referenced this pull request Feb 22, 2016
do not make inferences with the same source\target pair multiple times
@vladima vladima merged commit f029ae2 into master Feb 22, 2016
@vladima vladima deleted the cachePairs branch February 22, 2016 19:50
vladima added a commit that referenced this pull request Mar 3, 2016
do not make inferences with the same source\target pair multiple times
vladima added a commit that referenced this pull request Mar 3, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants