Skip to content

Why is this plugin slower than tsc compiler? #196

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

Closed
0xorial opened this issue Dec 26, 2018 · 27 comments
Closed

Why is this plugin slower than tsc compiler? #196

0xorial opened this issue Dec 26, 2018 · 27 comments

Comments

@0xorial
Copy link
Contributor

0xorial commented Dec 26, 2018

I have a webpack setup, where my complete compilation takes ~12 seconds and incremental compilation takes ~2s. I profiled it and noticed that it spends 1.5s in TypeScript compilation (which is in external process so I couldn't find how to run profiling on it). What surprises me a bit, is that it does not make a difference whether I change file with very few dependencies, or with lots.
TypeScript-wise all my setup is in tsconfig.json, so I tried to run tsc --watch. There initial compilation takes ~8s, but then incremental changes in files with few dependencies take ~0.2s, and in files with lots of dependencies take ~2s.
For more background, my project has ~150 .ts files with total size ~1MB.

Is there anything I can do to make my compilation faster? Or at least profile it somehow?

@johnnyreilly
Copy link
Member

You might want to follow some of the profiling work done by @jantimon in this issue: merkle-open/webpack-config-plugins#24

We welcome anything that speeds up compilation, so if you want to dig into it and try to make things better that'd be amazing!

@piotr-oles
Copy link
Collaborator

We didn't implement new API for incremental build that checks files dependencies - microsoft/TypeScript#20234 - that's the problem I think.

@jantimon
Copy link

jantimon commented Dec 27, 2018

Webpack has a large overhead for every increments build. (about 2/3 of the time one incremental build takes)

I proposed a fast lane which would skip building up a module tree and chunk optimizing as long as you don’t add or remove a dependency inside a source file.

The webpack team is open for the idea but it will take a lot of effort to implement it.

@johnnyreilly
Copy link
Member

Sounds like a great idea!

@0xorial
Copy link
Contributor Author

0xorial commented Dec 28, 2018

I've implemented a version which uses TypeScript API. Is there some bigger project which I could use to test it?

@johnnyreilly
Copy link
Member

Great work! If you could test it on your own projects that'd be useful. I'm travelling right now but I'd be happy to test it on a fairly chunky one I work with when I get back...

I'm on a phone so looking at the code is tricky. That being the case, I wonder if you can answer a question: How straightforward is swapping from using the watch API to using the existing one? Assuming all looks good I'd like to keep them existing in parallel until we know the new one is solid.

Again, well done for putting the effort in!

@0xorial
Copy link
Contributor Author

0xorial commented Dec 28, 2018

The numbers with my version are following: initial compile: ~12s, incremental changes in a file with few dependencies: ~0.6s, incremental changes in a file with lots of dependencies: ~3.5s (note, however, that I switched from 2 threads, to 1 thread, because of reasons which I will explain below).

Unfortunately, using TypeScript incremental API is a bit tricky: if you use out-of-box compiler code (i.e. mimic what tsc does) you get really good time, but as soon as you try changing anything, you quickly fall into a strange situation where it is still faster than full recompile, but still much slower (time is comparable to current compilation time in fork-ts-checker-webpack-plugin for my project and I wonder what time it will give for a larger one).

Because of that, what I did now is used the original code from compiler almost untouched and only intercepted it in a few (rather hacky) places to be able to use it. It works approximately as follows:

  • I start the compiler watch program (one inside TypeScript), providing a callback for diagnostic messages (and it is really the only place where I managed to get those messages :( )
  • watch program runs initial compilation and calls watchFile to watch files that it thinks are relevant. I intercept those calls and store the provided callback, but do not let them fire, in order to wait until we have a signal from webpack
  • when there is a signal from webpack to start compiling, I invoke intercepted TypeScript watch callback with all accumulated file changes
  • then I wait for compilation to finish and return diagnostics that TypeScript gives me through callback that I provided in the very beginning.

As you can see there is very little control over compiler right now.
There are 2 parts of fork-ts-checker-webpack-plugin which would be affected by that:

  1. Parallelism. If we try to distribute incremental changes across different processes, there may be some errors missed - in case when changed files depend on one another.
    Because of that I cannot even imagine how would it be possible to run incremental watching in parallel. On the other hand, it runs REALLY fast even for big codebases. (try 'yarn watch' workflow with vscode repository, for example).

  2. Vue files. For now I completely ignore them in my implementation. I could imagine a solution for that though: since we intercept all filesystem calls we could make TypeScript think those files only contain TypeScript code :)

@johnnyreilly
Copy link
Member

Nice work @0xorial! To your points:

Parallelism. If we try to distribute incremental changes across different processes, there may be some errors missed - in case when changed files depend on one another.

I'd recommend we either throw an error if a user attempts to run using the watch API and more than 1 worker. Either that or just fix to 1 worker when in watch API mode; possibly logging to console that this is necessary for the watch API.

Vue files. For now I completely ignore them in my implementation. I could imagine a solution for that though: since we intercept all filesystem calls we could make TypeScript think those files only contain TypeScript code :)

If you could look into this that'd be amazing. This plugin is used in the Vue CLI I understand and I'm keen Vue users get the best experience possible. 😄

cc @yyx990803

@jantimon I didn't quite understand your question?

@johnnyreilly
Copy link
Member

By the way, you could probably lift the Vue approach that ts-loader uses: https://github.com/TypeStrong/ts-loader/blob/master/src/index.ts#L63

@0xorial
Copy link
Contributor Author

0xorial commented Dec 30, 2018

I have added some tests, linting support (without auto-fixing though) and Vue support. But since I do not use Vue myself I only had results from the test cases, which are quite basic. So it would be nice if someone could try and give more feedback.
Meanwhile, the incremental compilation time on my project is < 0.4s. Yay!

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 31, 2018

Brilliant! I'm travelling right now and so just on my phone. But I'll be online towards the end of this week and I'll try and give this a test drive then.

Would you be interested in raising a pull request with your fork? It would be great to understand the differences between your fork and what is in place presently.

Assuming all is good - I'd like to talk about an approach for graduating this into the plugin. It would be tremendous if we could use feature flagging to allow the user to opt in to using the watch API functionality. (Also, users of an older version of TypeScript don't have a Watch API available to them.) If users don't supply useWatchAPI: true (feel free to make up a better name 😁) then they use the standard behaviour. If they do then they use the new watch behaviour.

We can make some noise about the new feature with Twitter / blog posts to get others to test it. Assuming there's no problems we should look to make using the watch API the default for people running a late enough version of TypeScript (2.9 I think).

We did a similar thing with a file caching feature in ts-loader which is now on by default: https://github.com/TypeStrong/ts-loader/releases/tag/v5.3.2

Does this sound good to you?

@0xorial
Copy link
Contributor Author

0xorial commented Jan 1, 2019

Sounds great to me :)

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 4, 2019

I've given this a test on the project I'm working on. The performance seems to be roughly the same as current performance; perhaps marginally better.

Incremental build takes 12.5 seconds typically. With the fork it's more like 11.5 seconds. So it's an improvement 😄 Alas not as big as I'd hoped but an improvement nonetheless!

Also the hooks seem to be broken. https://github.com/johnnyreilly/fork-ts-checker-notifier-webpack-plugin is not being triggered.

Actually I think that's a problem with the notifier itself. I'll look at that

@0xorial
Copy link
Contributor Author

0xorial commented Jan 5, 2019

Thanks for the update! Just out of curiosity - can you try to run 'tsc --watch' on that project to see if it would also produce similar timing? Also - how big is your project?

I am wondering because, as I mentioned earlier, incremental compilation on vscode project (which has ~20k files and 20 MB) takes ~1s...

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 5, 2019

I'll give it another whirl on Monday to try your suggestions.

Question: do I need to do anything to use the incremental watch API or is it the default on your fork? I just yarn link-ed to your fork and didn't tweak anything. If it's behind the feature flag we discussed then I haven't actually tested it at all!

@0xorial
Copy link
Contributor Author

0xorial commented Jan 5, 2019

xD yeah, it might be the case. You need to set useTypescriptIncrementalApi: true.

@johnnyreilly
Copy link
Member

Aha! I will try that 😏

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 7, 2019

Tested. With useTypescriptIncrementalApi: true incremental build time is still around the same time on my big build. 😢

I'm wondering if I'm testing like-for-like. Could you share your config please?

Mine looks like this:

        new ForkTsCheckerWebpackPlugin({
            tslint: true,
            workers: 1,
            checkSyntacticErrors: false,
            watch: ['./src'],
            useTypescriptIncrementalApi: false | true
        }),

@0xorial
Copy link
Contributor Author

0xorial commented Jan 7, 2019

Well, that is a bit suspicious indeed. The configuration looks correct though. To make sure the code is there try to set workers > 1 - you should see error saying that you have to use 1 worker.

Myself, I worked with it for a few days and what I noticed is a quite strange behavior: in a more synthetic test (recompiling one file with small modifications) results are quite consistent around 0.4s. However, when I change multiple files (but still not many), I often get something of order of 3s.

Overall, I still like the whole thing, because it allows me to use react-hot-reload with <1s feedback time.

@johnnyreilly
Copy link
Member

To make sure the code is there try to set workers > 1 - you should see error saying that you have to use 1 worker.

Will do. Are you using async: false BTW?

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 8, 2019

Am an idiot. I wasn't using your fork at all 😄

3rd time's a charm!

And the results are in: it's bloody fast. Incremental build drops from 12 seconds to 2.

Please submit a PR and let's start the process of getting this in!

@0xorial
Copy link
Contributor Author

0xorial commented Jan 8, 2019

12 seconds to 2

OMG! That's a mead on my eyes! :)

async: false

Currently I do, but actually I shouldn't. I have also added console.time(...) inside a plugin to measure actual type-checking time, which perhaps should be behind a feature flag and output through something more flexible than console.

PR coming ASAP :)

@johnnyreilly
Copy link
Member

Currently I do, but actually I shouldn't.

Why not? Isn't that exactly what you want in dev mode (i.e. why wait for webpack)

I have also added console.time(...) inside a plugin to measure actual type-checking time, which perhaps should be behind a feature flag and output through something more flexible than console.

👍 for feature flag.

PR coming ASAP :)

Nice!

@yyx990803
Copy link

Great job guys!

@0xorial
Copy link
Contributor Author

0xorial commented Jan 8, 2019

Why not? Isn't that exactly what you want in dev mode (i.e. why wait for webpack)

Yes, yes, I meant I am using 'async: false', but I should switch to 'async: true'

@jantimon
Copy link

jantimon commented Jan 9, 2019

Amazing work 👍

@johnnyreilly
Copy link
Member

Closing - see: https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.0-alpha.2

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

No branches or pull requests

5 participants