-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
deno test is slow (pref: Dynamic imports) #2789
Comments
Requesting all potential imports makes sense when you are compiler a program with TypeScript, as it needs to ensure that all possible code would be valid, is transpiled, and doesn't have any type issues.
It kind of sort of does, but that is really up to the compiler host to implement. Previous versions of the compiler did have some sort of caching, but one of the problems is that TypeScript doesn't know what the "filename" of a module is until it calls Also, when we moved from AMD to ESM in the compiler, we also moved from single file transpile to program transpile. This means that TypeScript now tries to resolve everything upfront. Going back to single file would mean that a module wouldn't need to be transpiled until it is requested from the runtime, but it does mean that lazily loading the compiler would need to be repeatedly done if there were lots of dynamic imports and if spun down, TypeScript would lose its state and likely have to request a whole new load of source files in order to do the compiling. |
Does is even make sense to type check dynamic imports? It's strictly runtime thing to import module dynamically and if there's type mismatch on TS level then final effect would be the same (error from TS compiler vs runtime error from V8). Maybe in case of dynamic imports we could use I can see potential problem in that scenario (please correct me if I'm wrong): output of |
I don't think it is strictly a runtime thing. People would expect to consume modules in a type safe way, especially if they are TypeScript modules and enforce the checking. Just having V8 throw runtime errors, you would argue why have TypeScript at all then. Plus not every TypeScript error would be a JavaScript runtime error. There should be no type direct emits in TypeScript. The emit should always be the same. The risk with |
Pasting conversation from Gitter so it won't get lost:
|
When merging deno_std into the main repo, I found that running "deno test" is prohibitively slow due to this dynamic import issue. As a consequence I could not simply add it as a normal integration test (cli/tests/integration_tests.rs) and had to instead add three new github action builds so that it could be run in parallel to the rest of the tests. See #3093 |
I was just thinking about this. Specifically related to
While spinning up the compiler should be quicker, and we need to tackle that, we still have a challenge with dynamic imports that aren't statically analysable. So to work around specifically this issue with
Two other thoughts of things we might want to do here:
Anyways we can do a lot more before spinning up the compiler performance actually becomes a serious bottleneck again for this. |
That's right, the problem is that building next files we spend most on analyzing the same modules over and over, see first comment describing this behavior.
Spinning up the compiler is actually pretty fast, the slowest part is actual compilation by TS ( |
Yeah, the problem with incremental is there is no way to serialize the state. There is building, which is still worth investigating but all of these assume the compiler keeps running with its state. I think it is going to be complex. The other option is that for nom statically analysable imports is that we do single module transpiles with no default lib checking. It increases the risks of unsound code being run, and throwing runtime errors, but if you are doing large amounts of dynamic imports it seems like an acceptable tradeoff. It just means TypeScript won't catch all potential type errors. |
With compiler upgrade (🔥) #3644 I think we can close this issue for now |
@bartlomieju Does #3344 still make a significant difference? |
@nayeemrmn good question, feel free to remove it and let's see CI test times |
Dynamic imports are still extremly slow :/
Thats something i observerd generally: workers and dynamic imports do not seem to be affected by |
@einicher please open a new issue with your problem's description |
During work on denoland/std#516 it became apparent that dynamic imports have performance problems. Those problems occur if you import TypeScript file that needs to be compiled.
Case study:
Let's focus on example of
deno_std
as it's a good testing subject. Test runner looks for all files matching certain pattern and imports them dynamically usingimport()
syntax.Let's look at
flags
module; it has around 10 test files.As you can see this file imports
test
andassertEquals
fromtesting
module andparse
fromflags
. Every test file inflags
module imports them. So when we encounter dynamic import of such file following steps occur:kv_short_test.ts
to be compiledkv_short_test.ts
and callsresolveModuleNames
for importsa) imported module is requested by TS compiler
b) Deno looks for that file and sends its contents to TS compiler
c) TS compiler processes the file and does points 4-5 for it
kv_short_test.ts
and dynamic import is resolved.The problem is that after compiling and importing
kv_short_test.ts
dynamic import for next file will go through exactly same steps. That means that for each file TS compiler will request and compile files liketesting/mod.ts
,testing/asserts.ts
andflags.mod.ts
. So if we have 10 test files in the module then TS compiler would compileflags
andtesting
modules 10 times.Note that dynamic imports use the same compiler pipeline as static imports.
Possible solution:
I'd expect TS compiler to have some kind of cache that it can request files from to avoid recompiling the same files again and again. We would wire to that cache and add an op that would look for already compiled module (the action that
TsCompiler.get_compiled_module
already does).CC @ry @kitsonk @piscisaureus
The text was updated successfully, but these errors were encountered: