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

Always call getCandidateForOverloadFailure #28564

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 16, 2018

This would make it so the command line compiler and services don't differ in the overload chosen.

@ghost ghost force-pushed the getCandidateForOverloadFailure branch from db2a202 to 1738ee0 Compare November 16, 2018 18:21
@sandersn
Copy link
Member

sandersn commented Feb 6, 2020

@weswigham Should we be worried about performance problems in services from merging this? resolveUntypedCall is way simpler than getCandidateForOverloadFailure. Unfortunately we don't have an easy way to measure perf differences for a PR there.

(@amcasey in case this is a scenario you want to take note of.)

@weswigham
Copy link
Member

Should we be worried about performance problems in services from merging this?

The reverse - you should be concerned that command line performance might suffer; the language service checker has always been using the more expensive getCandidateForOverloadFailure - that's how it still provides completions, even in the presence of errors; unlike the command line compiler, it doesn't immediately replace the result with any.

Since that's the concern, a simple perf test on this PR should bear out if that's really a problem.

@sandersn
Copy link
Member

sandersn commented Feb 6, 2020

Oh, yeah, I misread produceDiagnostics. I'll merge with master and run a perf test then.

@weswigham
Copy link
Member

weswigham commented Feb 6, 2020

Do note that this is technically a prerequisite for #28584 (which is probably why you're here), which we'd like to be able to take to reduce services memory usage~

@sandersn
Copy link
Member

sandersn commented Feb 6, 2020

Ha ha nope. I'm just going through Pall Mall by order of age. #28584 is next.

sandersn added a commit that referenced this pull request Feb 7, 2020
@sandersn sandersn mentioned this pull request Feb 7, 2020
@sandersn
Copy link
Member

sandersn commented Feb 7, 2020

Closing in favour of #36665 since I couldn't find the branch for this PR.

@sandersn sandersn closed this Feb 7, 2020
sandersn added a commit that referenced this pull request Feb 7, 2020
@RyanCavanaugh RyanCavanaugh deleted the getCandidateForOverloadFailure branch October 25, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants