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

TypeScript Language Service is slow to load projects over network file systems #16426

Open
vikerman opened this issue Jun 9, 2017 · 10 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@vikerman
Copy link

vikerman commented Jun 9, 2017

TypeScript Version: 2.3.4

We have our code on a networked/FUSE file system with different root directories for generated files and such. We use the node module resolution. To load a fairly simple Angular+Angular material+RxJS project(transitively ~1.7K .ts and .d.ts files) the language service takes around 24 seconds to resolve all modules and return diagnostics. We can see about ~50K file stats being made to resolve everything.

In our case we actually generate tsconfig.json for our editors through a build step and we actually create the list of all the files the project needs in the "files" section of tsconfig.json.

We built a custom solution that uses this files list to proxy the ServerHost in the Language service - to respond to fileExists and directoryExists by using the "files" list instead of hitting the file system a whole lot of times. This reduced the initial project load time to 4 seconds (and is mostly just the overhead of reading all the files) which is very much an

Here is a PR of the fix we have currently - https://github.com/vikerman/TypeScript/pull/1/files

It would be nice to have a cleaner solution for this in the language service itself (maybe as an option?). This would help any team who have a similar network/FUSE file system as their source repository.

@chuckjaz @alexeagle

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 10, 2017

So the idea is to always assume a file exists if it's explicitly listed? IIRC this used to be the old behavior but we stopped that because it was actually slower with Node's module resolution on local file systems.

@DanielRosenwasser DanielRosenwasser added the In Discussion Not yet reached consensus label Jun 10, 2017
@vikerman
Copy link
Author

So the underlying problem is that number of fstat-s in the node module resolution is too high and it is very visible on a network drive. Our current solution is by overriding file/directoryExists - but maybe you can suggest some other way to solve it also.

And we need a clean way to integrate with the language service so that we don't have to maintain a separate entrypoint/binary. The LS plugin mechanism (#11976) was insufficient because the base LSHost is already created with the sys ServerHost and there is no way to override it.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2017

Do you need the node module resolution? can you share a sample --traceResolution output, i would like to understand what modules are we looking for and where.

@vikerman
Copy link
Author

We need node module resolution because we use path mapping. Related - #11979

Will get the logs for the traceResolution shortly.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2017

path mapping is not tied to node module resolution.

@mhegazy mhegazy added the Bug A bug in TypeScript label Jun 14, 2017
@mhegazy mhegazy assigned ghost Jun 14, 2017
@mhegazy mhegazy added this to the TypeScript 2.5 milestone Jun 14, 2017
@DanielRosenwasser DanielRosenwasser removed the In Discussion Not yet reached consensus label Jun 14, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 14, 2017

@mhegazy the problem, IIRC, is that if you use "classic", you also get inappropriate file-statting in a different way when walking up the directory spine. I think a mix of moduleResolution: "none" and moduleResolution: "node" is needed, but @vikerman and the team can correct me if I'm wrong.

Basically:

  1. Loading from package.json and the like is potentially needed from path-mapped directories.
  2. Walking up each directory's node_modules is not needed.

@vikerman
Copy link
Author

  1. We actually don't need package.json logic also. We do depend on some minimal parts of the node module lookup like looking for /index (Not sure if classic has that one)
  2. Walking up for node_modules and @types are definitely not needed.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

Looking through the trace from @vikerman, we are doing too many lookups for imports in .d.ts file, e.g. rxjs importing other parts of rxjs. we should optimize that. that will not remove all lookups but should reduce them by a big factor.

If that does not work either, we can try to add a flag to limit file lookups, but i would rather do that as a last resort.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 3, 2017

I think we're going to have to push this to the 2.6 milestone until we can discuss any solutions with @mhegazy when he comes back.

@DanielRosenwasser
Copy link
Member

Just to keep the two linked together, there's also an issue tracking pluggable module resolution at #18896.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants