Fix 2 issues with experimentalWatchApi #1159
Conversation
|
This is an amazing write up @appzuka - I need a little time to digest this but I thought I'd share a few thoughts that occurred:
Awesome - if that is the case then maybe we can look into removing that obsolete code subsequent to this PR Don't sweat AppVeyor test failures. AppVeyor is probably going be dropped - see chat with @g-plane here: #1144 (comment) Incidentally I think the intent of this PR being to get us to correctness before speed is a good shout. |
|
I have spent a while looking at the watchApi code now and I still am not sure how it will reduce compile time, which is the reason it is being introduced. Does anyone have an example repo where it clearly provides a benefit? Otherwise, it seems to add greatly to the complexity of ts-loader without benefit. I understand how the watch api can speed up tsc, but webpack includes its own watch functionality so I'm not clear this is needed in ts-loader. |
|
Hey @appzuka
It's a good question; the rationale for expecting the watch API will add speed benefits is the difference it made to the TypeStrong/fork-ts-checker-webpack-plugin#198 It's possible there's reasons the speed gains achieved in the plugin can't be realised in the context of a loader. Honestly don't know for sure; but the speed benefits the plugin achieved suggests there may be gold in them thar hills. |
package-lock.json
Outdated
| @@ -0,0 +1,10000 @@ | |||
| { | |||
There was a problem hiding this comment.
Can we remove the package-lock.json please? I've nothing against npm but ts-loader uses yarn.lock and I want to avoid maintaining 2 separate lock files. Thanks!
|
This looks great @appzuka - could we remove the Also could you increment the version in the |
|
Thanks! Shipping https://github.com/TypeStrong/ts-loader/releases/tag/v8.0.2 |
| Asset Size Chunks Chunk Names | ||
| app.d.ts 11 bytes [emitted] | ||
| bundle.js 4.85 KiB main [emitted] main | ||
| Asset Size Chunks Chunk Names |
There was a problem hiding this comment.
This change seems incorrect from project references point of view. It should not write lib\tsconfig.tsbuildinfo file since there are errors in the build of project lib this will cause unknown issues as project cannot have tsbuild info file if there are errors. (not in 3.9 for sure)
There was a problem hiding this comment.
Thanks for commenting @sheetalkamat! Would you advise reverting this?
In a side note, @appzuka has been observing that the watch API support (which I believe you originally added 🤗) does not improve performance of ts-loader. This is surprising given that it provides a massive performance boost to fork-ts-checker-webpack-plugin - however that does seem to be the case.
I'd always hoped that the watch API usage would become the default way to run ts-loader. The idea being that it stayed behind the experimentalWatchApi flag until it was fully functional and outperforming the servicehost approach. Then we'd drop the old approach in favour of the watch API. However, that day has not come sadly! Due to various bugs and the performance issue we're now thinking about potentially removing it.
What are your thoughts? I'm mindful you put a good amount of effort into implementation in the first place (which we're super grateful for BTW!) And also, if there's a simple thing ts-loader is doing slightly wrong here that would make the watch API perform I'd still love to make that happen. Again I can't forget that fork-ts-checker-webpack-plugin got turbo charged from using the watch API so there's a good chance we're doing something sub optimal.
What do you think?
cc @andrewbranch in case there's anything he'd like to add to this discussion too
There was a problem hiding this comment.
Looking at the commit prior to this PR, in projectReferencesWatchRefWithTwoFilesAlreadyBuilt, patch2 is supposed to use the new number added to the helper in patch 1:
In the output bundle you can see the new number has been added:
But in the watchAPI version it does not:
And the same with the Composite_WatchApi
Also note that tsconfig.tsbuildinfo is not written to the patch2 output:
After the PR#1159 the output of patch2 correctly includes the new content and the tsconfig.tsbuildinfo:
Note that the new tsconfig.tsbuildinfo is in the top level directory not in the lib folder.
I was concerned that the PR changed the comparison test results but I believe the previous output was incorrect so the changes reflect the fixing of behaviour by the PR. The watchapi code is quite complex so there could be further issues which need to be fixed.
There was a problem hiding this comment.
@appzuka my bad.. i saw that tsbuildinfo as from lib project instead of root project. Change looks good.
There was a problem hiding this comment.
@johnnyreilly Yes watchAPI should be faster and should see benefit.. I am investigating performance for internal team that uses ts-loader with solution builder work.. i can take a issue with watch API but i will need concrete repro steps so i can investigate. Watch API is kind of similar to solution builder API (how it handles file changes etc) so current perf work i am doing might help not sure. In the mean time if you can get repro where not using watch api is faster than watch api, i will be better equipped to investigate it. without that its just analysing code which i might not have enough time to do.
There was a problem hiding this comment.
@sheetalkamat - thanks so much!
@appzuka you've bumped on the problems with the performance of the watch API the most as you've been looking into this. I'm bearing in mind your comment here:
This PR fixes these issues. When I use this on a real project however
experimentalWatchApiis very slow. WithoutexperimentalWatchApithe build time for a larger project is around 37 seconds but with the watchApi it jumps to 168 seconds. This PR increases the build time to 199 seconds. Using transpileOnly and for-ts-checker the build time is reduced to 23 seconds.
Would you be up for providing some kind of demo repo which demonstrates performance being better with the non watch API vs the watch API?
No worries if you can't - I can try and get to it later (after my holidays)
There was a problem hiding this comment.
To observe any performance change a large codebase will be needed. This will not just be a single file with many lines, but many files nested several levels, like a real, large project. I have created a repo which generates a codebase to test the various options. The repo is at:
https://github.com/appzuka/ts-loader-watchapitest.git
The Readme gives instructions to install and run and the results I get on my system.
Perhaps I have misunderstood how the new watchApi is supposed to work in ts-loader but I cannot see any reduction in build time at any stage. A simple build with expermentalWatchApi is over 2x the time using transpileOnly and fork-ts-checker-webpack-plugin and 30% slower than a normal build. Even incremental builds are much slower.
I love the --incremental feature of tsc, but it seems to me it is hard to integrate this into a webpack loader. Loaders should perform one simple task - they take the source text given to them by webpack and they return transformed text. ts-loader with transpileOnly does this very well.
Trying to implement the incremental behaviour means that the loader must consider the whole project every time webpack asks it to process every file. ts-loader handles this by caching the compiled files after the first ts file is built and recovering them from the cache for later files at the cost of greatly increasing the complexity of ts-loader.
It seems to me that incremental mode would be better implemented as a webpack plugin that compiles the entire project to a js-cache folder before webpack starts to compile the project. tsc would need to be called just once and Webpack would then build using the js files. This would still work in watch mode - tsc would update the files in the js-cache after a ts file was updated then webpack would recompile the project.
Apart from greatly simplifying the process, this would enable the performance gains of incremental mode to be used with webpack. If the js-cache persists between runs on the filesystem, subsequent builds will use the tsconfig.tsbuildinfo file to only recompile as necessary.
As a proof of concept I separated the tsc part of the build of a project from webpack. For my project I found that about 50% of the initial build time was taken by tsc and 50% by webpack. On the second build the tsc part dropped close to zero, as would be expected using incremental mode.
Perhaps fork-ts-checker-webpack-plugin works because it is a plugin - it runs just once for each build not once for every file.
There would be some disadvantages to this method. You could not use a webpack loader to transform the code before being processed by tsc. I assume fork-ts-checker-webpack-plugin suffers from this as well, but I doubt it is often a requirement in real code.
I could have a go at creating such a plugin. It would probably be separate from ts-loader.
There was a problem hiding this comment.
I have created a repo which generates a codebase to test the various options. The repo is at:
Thanks @appzuka! @sheetalkamat you should find what you need here ☝️
Props on your plugin experiment; the results sound very interesting! Incidentally, fork-ts-checker-webpack-plugin is part of the TypeStrong family; if you fancied applying some of your experiments there @piotr-oles and myself may well be grateful 🤗
You could not use a webpack loader to transform the code before being processed by tsc. I assume fork-ts-checker-webpack-plugin suffers from this as well, but I doubt it is often a requirement in real code.
Funnily enough I think this was an issue and has been resolved.... I've been digging through recent PRs but I can't find the one in question. Pretty sure it's out there though!
I created a repo to test the experimentalWatchApi and demonstrate 2 issues present in ts-loader@8.0.1:
https://github.com/appzuka/ts-loader-watchapi-test.git
I have detailed the issues in the README but in summary:
This PR fixes these issues. When I use this on a real project however experimentalWatchApi is very slow. Without experimentalWatchApi the build time for a larger project is around 37 seconds but with the watchApi it jumps to 168 seconds. This PR increases the build time to 199 seconds. Using transpileOnly and for-ts-checker the build time is reduced to 23 seconds.
I am submitting this PR as it stops ts-loader from emitting the wrong js in these 2 cases so I hope it moves this option closer to being production ready even if it still seems some way off.
If someone has a repo where experimentalWatchApi does improve the build time I will investigate why my project shows such a huge slowdown.
Issue 1 - Loader
For the loader issue, when getEmitFromWatchHost processes the first file (index.ts) the builderProgram compiles both index.ts and also render.ts, which it depends on. Both of these are added to the outputFiles of the watchHost.
ts-loader/src/instances.ts
Line 692 in cd59216
Only the source for index.ts was the one passed from webpack as it was added to the cache in successLoader. The source used to compile render.ts was read from the filesystem directly. Therefore the output for render.ts which was stored in the watchHost was without the loader applied.
Later, when webpack calls ts-loader to compile render.ts, it gets the output which was saved in watchHost.outputFiles previously. The loader is only applied when render.ts is edited and saved, which forces webpack to pass the source after processing by the loader to ts-loader where it is added to the cache in successLoader.
The solution in this PR is to only save the output when the sourcefile was passed from webpack. This will mean that the file may be processed multiple times by typescript before it is cached so could incur a time penalty, but that is better than skipping the loaders and emitting the wrong code.
I note that this solution appears to make obsolete code elsewhere to run source through the loaders, such as that added by PR#1115.
Issue 2 - Files not updated after the first edit
In index.ts there is a function updateFileInCache. If the current file contents are different from the contents passed to the loader by webpack then the file is updated in the cache and fileWatcherEventKind is changed to instance.compiler.FileWatcherEventKind.Changed
ts-loader/src/index.ts
Line 372 in cd59216
This triggers a sequence of events which leads to the changed file being emitted to webpack.
After the initial run, watch-run.js starts to watch the files. On the first update the watcher updates all files in the project in its cache through the updateFileWithText function. This is run for the first time on the first change after the initial build in watch mode and it is run before ts-loader. This is a problem because when ts-loader then runs the updateFileInCache function above it will not detect a difference between the file contents passed by webpack and the file text in the cache because it was already updated just before ts-loader was called.
To solve this I have added the following code to updateFileInCache:
With this change this repo updates every time one or more of the TS files are changes, as expected.
There may be alternative ways to fix these problems so if people familiar with the code want to suggest alternatives that would be great.
I see some errors running comparison tests. For example, in
projectReferencesWatchRefWithTwoFilesAlreadyBuilt_Composite_WatchApi the output is:
Instead of
Looking at the actual app.ts for patch2 the new output is correct. Also, this brings the output of the tests with WatchApi into line with those without, as it should. There were 6 comparison tests which needed the expected output to be changed, all using WatchApi.