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

TS compiler loads modules synchronously #2626

Closed
bartlomieju opened this issue Jul 9, 2019 · 4 comments · Fixed by #2949
Closed

TS compiler loads modules synchronously #2626

bartlomieju opened this issue Jul 9, 2019 · 4 comments · Fixed by #2949

Comments

@bartlomieju
Copy link
Member

bartlomieju commented Jul 9, 2019

This issue came up when working on rewrite of DenoDir and is a blocker for v1.0 (#2473)

In the current setup TS compiler requests module synchronously one-by-one.
Modules are requested in two separate (synchronous) functions:

  • resolveModuleNames - which is called for all imports in given TS file, compiler wants to know fully resolved URL to that module, but we're actually downloading the module in this step by issuing op_fetch_module_meta_data (synchronous).
  • getSourceFile - this function returns source code of module. Since op_fetch_module_meta_data was already called in resolveModuleNames the files are already downloaded.

Ideas for solutions:

  • During resolveModuleNames I wanted to send multiple specifiers to Rust to kick off parallel download of modules and block on it, but @ry suggested it's not possible since it's sync op.
  • During resolveModuleNames send one message to Rust for each specifier kicking off download of that module in background - this could be represented as ModuleMetaDataFuture that is stored in DenoDir. During subsequent getSourceFile call that future should be resolved. Although I'm not sure if it doesn't present the same problem as above - future won't be polled because it's a sync op.
  • Explore TypeScript compiler API to find async alternatives to resolveModuleNames and getSourceFile

CC @ry @kitsonk

@ry ry mentioned this issue Jul 9, 2019
43 tasks
@kitsonk
Copy link
Contributor

kitsonk commented Jul 10, 2019

I don't think you are going to get the TypeScript compiler to work asynchronously. It is sync throughout, which has to do a lot with the way the underlying hosts have to behave and the way the analysis has to happen. There are no async APIs.

Ref: microsoft/TypeScript#29361

The only possible path is point two, but I would say that you would send 1 message to kick off the loading. TypeScript just needs to know what the resolved module name is, so when it goes to getSourceFile it knows specifically what to request. The distance between those two calls is very short though, so I am not sure there would be any significant performance improvement. Even then the compiler isn't going to store up all of its modules and request them all in resolveModuleNames, it will be statically analysing each dependency, identifying the next dependency it doesn't have and requesting those in batches on a per module basis. It doesn't know it needs module C for example until it has loaded module A, which depends on module B, loads that, which loads module C.

How is it really a blocker for the rewrite, other than behaving differently than you would like it?

@bartlomieju
Copy link
Member Author

I don't think you are going to get the TypeScript compiler to work asynchronously. It is sync throughout, which has to do a lot with the way the underlying hosts have to behave and the way the analysis has to happen. There are no async APIs.

Ref: microsoft/TypeScript#29361

Thanks for pointing this out, after quick scan of TS compiler API I felt that it might not be possible to make it async.

The only possible path is point two, but I would say that you would send 1 message to kick off the loading. TypeScript just needs to know what the resolved module name is, so when it goes to getSourceFile it knows specifically what to request. The distance between those two calls is very short though, so I am not sure there would be any significant performance improvement. Even then the compiler isn't going to store up all of its modules and request them all in resolveModuleNames, it will be statically analysing each dependency, identifying the next dependency it doesn't have and requesting those in batches on a per module basis. It doesn't know it needs module C for example until it has loaded module A, which depends on module B, loads that, which loads module C.

That's correct. I'm okay with compilation being synchronous but if downloading of modules can be parallelized we should definitely explore this possibility - and since resolveModuleNames requests batch of files it might be possible to do that. I've started working on prototype implementation for that, I'll get back to you with the results shortly.

How is it really a blocker for the rewrite, other than behaving differently than you would like it?

It's not a blocker for rewrite - just this issue was uncovered during rewrite, but in conversation with @ry we agreed that downloading of modules should be somehow optimized before 1.0.

@ry
Copy link
Member

ry commented Jul 10, 2019

It's unfortunate that typescript doesn't support async module loading.

I wonder if ts.installPackage() could somehow be used. Do anyone know what this does?
https://github.com/denoland/deno_third_party/blob/e319136a1744710ffd3c2ea372a7dfc1d462f120/node_modules/typescript/lib/typescript.d.ts#L4781

Otherwise using resolveModuleNames to queue up async download would still be helpful. If A imports B, C, D. We currently download B, C, and D serially. We could return the module name synchronously, but start the download in the background. Later we would actually wait on the B download, but at least C and D would still be downloading.

I think this is important to consider during the refactor of DenoDir.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 11, 2019

Yeah, that is part of the Automatic Type Acquisition system, where TypeScript will go off and request of VSCode to install a type definition (@types/). Because it is async request in nature, they had to implement it that way, but the didn't re-architect the rest of the compiler.

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 a pull request may close this issue.

3 participants