Watch mode: rebuild takes too long after adding a simple console.log to file (~2 minutes)
#42173
Comments
RyanCavanaugh
added
the
Needs Investigation
|
I tested this again and the rebuild took closer to 1.5 minutes than 2 minutes (TypeScript 4.1.2). I then upgraded to TypeScript 4.2 RC and repeated the test, and it seemed like there was no improvement. I don't know how realistic this is but I would like to see rebuilds in the single digits, especially if I'm just adding a |
|
This is because the d.ts emit takes long time.. The project in question Below shows the timings: |
sheetalkamat
added
Design Limitation
|
Just chatted with @amcasey offline and here is the link to analyze the trace https://www.npmjs.com/package/@amcasey/ts-analyze-trace but it might not show anything apart from the fact that d.ts is slow.. He will be looking into improving analysing which type causes slow conversion but thats not yet part of it |
|
At present, that tool specifically only outputs information about check time. However, it builds on an underlying tracing system that will show tracing time. Unfortunately, it nearly always just shows that many files took a small amount of time each. I've been working on figuring out how to pinpoint problematic input code. |
|
Do you expect the changes in 4.3 beta (#43314) will help improve this issue at all? |
|
Don’t think so .. at least for the scenario you shared .. because at some point you do need .d.ts calculation and that’s where the issue lies |
|
I spent time trying to enhance the tracing code for another project with slow emit and found more or less what I expected - that tracing is not well-suited to identifying why emit is slow. Basically, tracing is fundamentally about finding individual expensive calls. When cost is spread across lots of little calls with the same stack, it's more effective to use a profiler. Nevertheless, I added a bunch of tiny tracepoints (and disabled sampling), in case there was something I could learn by aggregating. Some observations:
My personal, but unverified, suspicious is that types that require an @OliverJAsh I don't have access to your sample repo. It's not urgent, since I don't expect the results to be different than for the repos I've already looked at, but I'm happy to confirm, if you're interested. first.last@microsoft |
|
@amcasey You should now have access.
Any guidance on this would be really appreciated. I've tried to optimise declaration size and emit time using the tricks mentioned in #34119 but this is as good as I could get it. I wish we didn't have to resort to these tricks. |
|
@OliverJAsh do you mind trying #41219 to see if that is helping in your scenario.. That PR will skip calculating d.ts all together and assume that all your changes are always affect your dependency tree. so there is some trade off with that PR but given that in your code time spent in d.ts is larger even for non local change it might be acceptable.. Just a thought.. we are weighing benefits of that PR and option so your feedback would be helpful. #41219 (comment) has installable build from master with the change Thanks |
|
Also #43973 might be relevant to you in case its some kind of visibility calculation taking longer |
|
@OliverJAsh Step one for getting rid of the tricks is working with engaged community members to identify best practices we can identify and automate. ;) Looks like you're going to hit some snags when you move to TS 4.2 because of the DOM updates. You'll need to update @types/resize-observer-browser. I'm starting with a full build because that's easier to reason about than an incremental build. I'm already seeing 10 or files that each take 2-3 seconds to emit, which is crazy. I'll investigate. Are you okay with me posting paths and sketchy details in this issue or do you want to discuss project-specific issues over email (you have mine)? Edit: There are 5 files emitted slowly and they're each emitted twice. (?!) |
|
Incidentally, you're spending about 1.5 seconds checking @babel/types. If you don't have a good reason not to, you might want to enable |
|
@sheetalkamat Individual .d.ts files are being emitted more than once (and I'm pretty confident the second one isn't a no-op since it also takes multiple seconds). Expected? First stack Second stack Line numbers are from |
|
Yes and we have todo on investigation on caching the declaration emit but haven’t gotten to it .. One is to get the signature of the file while other is actual emit .. I think I had looked this and we needed to store some diagnostics and emit etc as well as need to handle custom transformers and other api things which makes it not so trivial and hence it was postponed for future |
We have upgraded to TS 4.2 since I originally posted this issue. I have pushed a new branch with
That's fine!
Here's a branch with that change: Unfortunately, when I save a file (step 5/6 in my original issue description at the top), I get this error: |
|
@OliverJAsh to give an example of locally optimizing emit, if you export How did I figure that out? Is it ugly? Very much so - you would only do this after confirming you have a performance problem. Should the compiler do this for you? I would argue not - what you export and don't export is an important design decision and the compiler shouldn't be making those for you. There is, however, room for another tool that would warn you when this happens and point out opportunities. I'm currently exploring how we could discover cases like this automatically. |
Thank you for reporting this.. have created #43986 to fix that part |
|
@OliverJAsh Some high-level observations:
I grabbed the bug to investigate the tooling and I think I've taken that as far as I can at the moment, so back to @sheetalkamat to look at watch mode issues. |
|
As i said this is so far design limitation from watch perspective because .d.ts emit is the cause of slowness and you should see that if you improve your .d.ts emit perf by taking into account what andrew has suggested watch will get better too.. |
sheetalkamat
removed
the
Needs Investigation
|
Thank you for investigating this and sharing your notes!
Unionize is a library that provides syntax sugar for defining tagged unions with their constructors, predicates and match functions. It uses some clever type arithmetic to accomplish this. The alternative is writing all of this by hand which would be much simpler from a type perspective but significantly more boilerplate, so it's a trade off. Given how frequently we use tagged unions in our application, we can't really live without it. Note that the popular library io-ts is doing similar things and there will be more to come in the next major version.
I'm not sure I follow what you're suggesting here. What would the fix look like?
Thanks. I've upgraded my
Questions
|
Replacing an object type with a named interface. As I think I mentioned, it may be a step backwards for maintainability, but it should be a build-perf win.
Are you sure the middle one wasn't an incremental build? That's suspiciously fast. |
I tested it twice just to double check. Each time I ran my test (following the steps in my original post above) I deleted the build folder to ensure there was no cache. |
RyanCavanaugh
added
the
Rescheduled
|
Is there an option for not checking the types and just run the app without typechecking? |
|
@sahin52 If you just want to transpile away the type annotations so you can run, you might want to try something like esbuild. |
Bug Report
🔎 Search Terms
performance, tsc, watch, build, rebuild, declaration, emit, incremental
🕗 Version & Regression Information
This is the behavior in every version I tried, and I reviewed the FAQ for entries about performance.
⏯ Playground Link
Unfortunately I cannot provide a reduced test case as I can only reproduce this on our very large project.
💻 Code
TypeScript 4.1.2
typescript-watch-issue. This is a private repository but I believe some members of the TS team already have access from debugging previous issues. If you don't have access, please send me your email address so I can provide access (TS team members only).yarnto install TStsc --build ./tsconfig.json --watchapp/actions/index.tsand add this line anywhere in the file, e.g. at the very bottom:console.log('foo')tscto rebuild🙁 Actual behavior
The rebuild takes too long. On my machine it takes over 2 minutes.
🙂 Expected behavior
I expected the rebuild to be very fast, because we're using all of the performance optimisations at our disposal: e.g.
incrementaland project references.One thing I noticed that might be helpful for debugging: the generated declaration file for
app/actions/index.ts—target-tsc-build/app/actions/index.d.ts—is fairly small after the first build (after step 4) but then for some reason it is significantly larger after the second build (after step 6). I don't understand why these declarations would change after such a simple change.Subsequent changes to this file do not suffer from this problem, e.g. if we remove the
console.log('foo')line that was added in step 5 and then save the changes to trigger another rebuild, the rebuild is very fast.The text was updated successfully, but these errors were encountered: