The Wayback Machine - https://web.archive.org/web/20191017142106/https://github.com/angular/angular/pull/32479
Skip to content

/angular

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

fix(language-service): use tsLSHost to do module resolution #32479

Closed
wants to merge 1 commit into from
+68 −16

Conversation

@kyliau
Copy link
Member

commented Sep 4, 2019 •
edited by ayazhafiz

This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for every program change (which means every
few keystrokes trigger a massive number of fs calls).

There are two solutions here:

  1. Provide a cache to ts.resolveModuleName()
  2. Use ts.LanguageServiceHost.resolveModuleNames()

Since every Project (and by extension ConfiguredProject) implements
ts.LanguageServiceHost interface, (2) is the preferred solution here.
i.e. the TypeScript LanguageServiceHost always has the
resolveModuleNames() defined even though it's optional in the interface.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@kyliau kyliau requested a review from angular/tools-language-service as a code owner Sep 4, 2019
@ngbot ngbot bot modified the milestone: needsTriage Sep 4, 2019
@googlebot googlebot added the cla: yes label Sep 4, 2019
@kyliau kyliau force-pushed the kyliau:cache_module_resolver branch 2 times, most recently from e12f2ae to d04ab57 Sep 4, 2019
@kyliau kyliau removed the PR state: WIP label Sep 4, 2019
@kyliau kyliau requested a review from ayazhafiz Sep 4, 2019
@ayazhafiz
ayazhafiz approved these changes Sep 4, 2019
View changes
@kyliau

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Sep 4, 2019

@andrius-pra you would be interested in this! :)
@ngbot ngbot bot added the PR action: merge label Sep 4, 2019
@ngbot

This comment has been minimized.

Sign in to view
Copy link

commented Sep 4, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: test_docs_examples_ivy" is failing
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.
@kyliau

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Sep 4, 2019

caretaker: failure in test_docs_examples_ivy is unrelated to this change.
kyliau added a commit to kyliau/angular that referenced this pull request Sep 4, 2019
@kyliau
fix(language-service): use tsLSHost to do module resolution
7c143bb 
This is a patch PR for angular#32479

This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for every program change (which means every
few keystrokes trigger a massive number of fs calls).

There are two solutions here:

Provide a cache to ts.resolveModuleName()
Use ts.LanguageServiceHost.resolveModuleNames()
Since every Project (and by extension ConfiguredProject) implements
ts.LanguageServiceHost interface, (2) is the preferred solution here.
i.e. the TypeScript LanguageServiceHost always has the
resolveModuleNames() defined even though it's optional in the interface.
@kyliau kyliau referenced this pull request Sep 4, 2019
Closed
2 of 14 tasks complete
@andrius-pra

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Sep 5, 2019

@kyliau, I have compiled language service and tested with angular/vscode-ng-language-service@next. I'm getting these errors in console logs:

Could not resolve module '@angular/core' relative to file undefined
Could not resolve module './core' relative to file d:/github/angular/aio/node_modules/@angular/core/core.d.ts
Could not resolve module 'rxjs' relative to file d:/github/angular/aio/node_modules/@angular/core/core.d.ts
Could not resolve module '@angular/router' relative to file undefined
Could not resolve module './router' relative to file d:/github/angular/aio/node_modules/@angular/router/router.d.ts
Could not resolve module '@angular/core' relative to file d:/github/angular/aio/node_modules/@angular/router/router.d.ts
Could not resolve module '@angular/common' relative to file d:/github/angular/aio/node_modules/@angular/router/router.d.ts
Could not resolve module 'rxjs' relative to file d:/github/angular/aio/node_modules/@angular/router/router.d.ts

and also the language server crash here (tsserverlibrary.js):
image

my environment:
windows 10
node v10.16.3
angular/vscode-ng-language-service ( compiled from 1439441)
@ayazhafiz

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Sep 5, 2019 •
edited

I am seeing the issue @andrius-pra noted as well. This does not seem to be an issue with 1439441 ; it is repro without that commit.
@kyliau
fix(language-service): Use module resolution cache
Loading status checks…
5a30406 
This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for **every** program change (which means every
few keystrokes trigger a massive number of fs calls).
@kyliau kyliau force-pushed the kyliau:cache_module_resolver branch from d04ab57 to 5a30406 Sep 9, 2019
@matsko matsko closed this in 6052b12 Sep 10, 2019
kyliau added a commit to kyliau/angular that referenced this pull request Sep 10, 2019
@kyliau
fix(language-service): cache module resolution
cb7b0e2 
This is a patch PR for angular#32479

This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for every program change (which means every
few keystrokes trigger a massive number of fs calls).
matsko added a commit that referenced this pull request Sep 12, 2019
@kyliau @matsko
fix(language-service): cache module resolution (#32483)
Loading status checks…
1c5b157 
This is a patch PR for #32479

This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for every program change (which means every
few keystrokes trigger a massive number of fs calls).

PR Close #32483
@kyliau kyliau referenced this pull request Sep 12, 2019
Open
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
@kyliau @arnehoek
fix(language-service): Use module resolution cache (angular#32479)
d821929 
This PR fixes a critical performance issue where the language
service makes a MASSIVE number of filesystem calls when performing
module resolution.
This is because there is no caching. To make matters worse, module
resolution is performed for **every** program change (which means every
few keystrokes trigger a massive number of fs calls).

PR Close angular#32479
@angular-automatic-lock-bot

This comment has been minimized.

Sign in to view
Copy link

commented Oct 11, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.
@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 11, 2019
@kyliau kyliau deleted the kyliau:cache_module_resolver branch Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ayazhafiz ayazhafiz

@angular/tools-language-service tools-language-service

Assignees
No one assigned
Labels
PR action: merge PR target: master-only cla: yes comp: language-service
Projects
None yet
Milestone
  needsTriage
4 participants
@kyliau @andrius-pra @ayazhafiz @googlebot
You can’t perform that action at this time.