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

Speed up Constraint Solver's overload resolution (II) #1650

Merged
merged 3 commits into from
Nov 7, 2016
Merged

Speed up Constraint Solver's overload resolution (II) #1650

merged 3 commits into from
Nov 7, 2016

Conversation

gusty
Copy link
Contributor

@gusty gusty commented Oct 23, 2016

This will speed up to 2x overload resolution in cases like this.

This PR is a follow-up of #1530 where it was discussed that ideally the constraint solution can be committed a bit early.

The problem was that knowing the solution early would make some very exotic cases like this one to fail.

After analyzing the problem I found that cases like that would fail because the first overload that is considered would succeed if the constraint is assumed as solved, when in fact it should fail (false positive).

Assuming that specific constraint solution makes impossible to solve the nested overload resolution called by SolveMemberConstraint which is expected to fail and indeed it fails, but because that result is ignored, it's marked as a false positive and the compilation breaks later by not being able to decide between 2 overloads: the good one and the bad one which is treated as good.

The proposed change adds an internal exception to signal the calling function (in the analysed case it was called from solveTypMeetsTyparConstraints) an unresolved overload and make the whole assumption fail.

The tests I made show no performance degradation but an improvement in some cases, which is logically expected because we will be using a shortcut in some cases.

But the improvement went a bit further since now by signaling an inconsistent case it also discards early some paths and I wouldn't be surprised if it fixes some existing bugs which were consequence of this previously ignored result.

Here's a test I run with the following results:

(* before #1530 *)  val compileTime : float = 20613.1785
(* after #1530 *)   val compileTime : float = 563.0135
(* after this PR *) val compileTime : float = 280.0141

* Commit constraint solution before the type equality test.
* SolveMemberConstraint will signal through an internal exception that it failed due to an unresolved overload.
@gusty
Copy link
Contributor Author

gusty commented Oct 24, 2016

The only test that was failing is the example added by @forki on #1530 which was never finishing to compile before that PR, then after #1530 it fails with "No overloads match".

Now with this PR it will fail with "This value is not a function ..." which could makes sense by reading the code, since it's a () value

So this could be an example of a bug fix mentioned above in my PR description. Before it was considering a false positive which gave more than one valid overload, that's why it was failing with "No overloads match".

@dsyme
Copy link
Contributor

dsyme commented Nov 1, 2016

This change looks great. Approved.

@gusty
Copy link
Contributor Author

gusty commented Nov 4, 2016

@dsyme I found a small issue with this PR but it's fixed now.

The original reasoning was to observe the result only when calling from solveTypMeetsTyparConstraints.

Intuition and extensive testing showed that permitWeakResolution could be used for this purpose however I just came up with a very exotic snippet that will fail.

To avoid that, we need to pass an additional parameter which was my original reasoning, and that's exactly what I did to fix it.

@KevinRansom KevinRansom merged commit 0c37abe into dotnet:master Nov 7, 2016
@KevinRansom
Copy link
Member

Thank you for taking care of this

KevinRansom pushed a commit that referenced this pull request Jan 4, 2018
forki pushed a commit to forki/visualfsharp that referenced this pull request Jan 20, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074)

* Add option to toggle unused declarations analyzer

* Better name and handle registering code fixes.

This will ensure that if someone uses warnon:1182, we won't suggest
fixes if they've turned off the feature.

* Revert dotnet#1650 (and dotnet#3366) (dotnet#4173)

* Fix error logging in brace matching code (dotnet#4140)

* Remove error logger pushing code

* Update service.fs

* Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202)

* LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194)

* Fixed FCS netcore tests (dotnet#4180)

* Remove ambiguous resolution error FS0332 (dotnet#4170)

* Add IsInteractive to parsing options for script load closures (dotnet#4169)

* Add IsInteractive to FSharpParsingOptions

* Add test

* Set serializable bit for all serializable types (dotnet#4211)

* Minor fix (dotnet#4195)

on string 58.

*  Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222)

* Symbols API: add Index to active pattern case, Name to pattern group

* Symbols API: add active pattern case use tests

* don't rebuild (dotnet#4230)

* Optimize in FCS

* Transport tcConfig

* Cleanup

* Replace more ILAsm in Exprs

* More ILAsm replacements

* update resource name

* Added some tests
ncave added a commit to ncave/fsharp that referenced this pull request Jan 24, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074)

* Add option to toggle unused declarations analyzer

* Better name and handle registering code fixes.

This will ensure that if someone uses warnon:1182, we won't suggest
fixes if they've turned off the feature.

* Revert dotnet#1650 (and dotnet#3366) (dotnet#4173)

* Fix error logging in brace matching code (dotnet#4140)

* Remove error logger pushing code

* Update service.fs

* Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202)

* LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194)

* Fixed FCS netcore tests (dotnet#4180)

* Remove ambiguous resolution error FS0332 (dotnet#4170)

* Add IsInteractive to parsing options for script load closures (dotnet#4169)

* Add IsInteractive to FSharpParsingOptions

* Add test

* Set serializable bit for all serializable types (dotnet#4211)

* Minor fix (dotnet#4195)

on string 58.

*  Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222)

* Symbols API: add Index to active pattern case, Name to pattern group

* Symbols API: add active pattern case use tests

* don't rebuild (dotnet#4230)

* Optimize in FCS

* Transport tcConfig

* Cleanup

* Replace more ILAsm in Exprs

* More ILAsm replacements

* update resource name

* Added some tests
ncave added a commit to ncave/fsharp that referenced this pull request Jan 24, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074)

* Add option to toggle unused declarations analyzer

* Better name and handle registering code fixes.

This will ensure that if someone uses warnon:1182, we won't suggest
fixes if they've turned off the feature.

* Revert dotnet#1650 (and dotnet#3366) (dotnet#4173)

* Fix error logging in brace matching code (dotnet#4140)

* Remove error logger pushing code

* Update service.fs

* Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202)

* LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194)

* Fixed FCS netcore tests (dotnet#4180)

* Remove ambiguous resolution error FS0332 (dotnet#4170)

* Add IsInteractive to parsing options for script load closures (dotnet#4169)

* Add IsInteractive to FSharpParsingOptions

* Add test

* Set serializable bit for all serializable types (dotnet#4211)

* Minor fix (dotnet#4195)

on string 58.

*  Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222)

* Symbols API: add Index to active pattern case, Name to pattern group

* Symbols API: add active pattern case use tests

* don't rebuild (dotnet#4230)

* Optimize in FCS

* Transport tcConfig

* Cleanup

* Replace more ILAsm in Exprs

* More ILAsm replacements

* update resource name

* Added some tests
forki added a commit to forki/visualfsharp that referenced this pull request Jan 31, 2018
dsyme added a commit to dsyme/fsharp that referenced this pull request Feb 13, 2018
dsyme added a commit to dsyme/fsharp that referenced this pull request Feb 13, 2018
dsyme added a commit that referenced this pull request Feb 13, 2018
KevinRansom pushed a commit that referenced this pull request Feb 17, 2018
* Optimize in FCS

* Transport tcConfig

* Cleanup

* Replace more ILAsm in Exprs

* More ILAsm replacements

* update resource name

* Added some tests (#40)

* Add option to toggle unused declarations analyzer (#4074)

* Add option to toggle unused declarations analyzer

* Better name and handle registering code fixes.

This will ensure that if someone uses warnon:1182, we won't suggest
fixes if they've turned off the feature.

* Revert #1650 (and #3366) (#4173)

* Fix error logging in brace matching code (#4140)

* Remove error logger pushing code

* Update service.fs

* Fix #4200: Vsix: fix empty "New file" window for web projects (#4202)

* LOC CHECKIN | visualfsharp - master | 20180112 (#4194)

* Fixed FCS netcore tests (#4180)

* Remove ambiguous resolution error FS0332 (#4170)

* Add IsInteractive to parsing options for script load closures (#4169)

* Add IsInteractive to FSharpParsingOptions

* Add test

* Set serializable bit for all serializable types (#4211)

* Minor fix (#4195)

on string 58.

*  Symbols API: add Index to active pattern case, Name to pattern group (#4222)

* Symbols API: add Index to active pattern case, Name to pattern group

* Symbols API: add active pattern case use tests

* don't rebuild (#4230)

* Optimize in FCS

* Transport tcConfig

* Cleanup

* Replace more ILAsm in Exprs

* More ILAsm replacements

* update resource name

* Added some tests

* test conditions update

* test update

* test condition update

* test update

* review update

* added checked operators

* fixed dual conversions

* review fixes

* more targeted replacements

* adapt to latest

* added more tests

* added more tests

* review fixes

* fixed warnings
TIHan added a commit to TIHan/visualfsharp that referenced this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants