[Performance] Very long Typescript compile time #1135
Comments
|
Hi @moufmouf ! Thanks for linking to the typescript issue, and also relay adding the I had removed That said, I'm not against changes to our output that would make tsc go faster... I.e. reading Ryan's comment on These sort of "use Maybe we start there, and see how the After that, happy to look at adding |
In oneof scenarios, this commit proposes generating "else if" statements where applicable. The original idea was an attempt to speed up Typescript compilation by reducing the amount of paths Typescript can analyze. Spoiler alert => nothing changes from that point of view. The compilation of my [proto file](https://github.com/workadventure/workadventure/blob/develop/messages/protos/messages.proto) takes between 41 and 44 seconds with and without "else if" statements. Still, the "else if" conditions should slightly speed up the runtime execution, by avoiding unnecessary checks to the Javascript runtime. This PR is directly linked to #1135 Note: in #1135, @stephenh you recommend trying to use a switch statement (instead of "else if"). This would require a really big refactoring and I'm not familiar enough with the code base to attempt that. The "else if" implementation was a bit of a low hanging fruit. I still think the "//ts-nocheck" annotation would be really useful and is the way to go, since it would simply take "0 seconds" to typecheck the generated file (you can't beat 0 in terms of performance 😆 )
|
@moufmouf did you see this approach of putting Does that still work? |
|
Hey @stephenh , Yes, I've seen the comment and already tested it. It doesn't exactly work in the latest version. If I put in the proto file: it becomes in the Typescript file: /** @ts-nocheck */This syntax is not recognized by Typescript. So no luck this way. Currently, compiling my file without "ts-nocheck" takes 40 seconds, vs 10 seconds with "ts-nocheck" (10 seconds is still a lot but a 4x speedup). For my own use case, I can write a small script to prepend ts-nocheck just after the proto generation, but I still think the whole community would benefit from this change by default, since there is no reason the code you generate would suddenly contain an error. |
|
I gave this another shot and managed to implement the "switch case" solution instead of the "else if" solution. It solves the performance issue! Opening a PR right now! |
|
Ah gotcha. Fwiw I opened up the example https://github.com/workadventure/workadventure/blob/develop/messages/protos/messages.proto Which makes the "40 seconds for tsc" even more bizarre. You do use (...I had this reply typed up, and then just saw your latest comment! Amazing!) |
…ypescript performance (#1142) This is a follow-up (and solution) to #1135 and #1141 I generated a trace from the build process and using [typescript-analyze-trace](https://github.com/microsoft/typescript-analyze-trace) to analyze the hotspots. It turns out that time is spent almost exclusively in this kind of code: ``` if (object.choice?.$case === "aNumber" && object.choice?.value !== undefined && object.choice?.value !== null) { message.choice = { $case: "aNumber", value: object.choice.value }; } else if ( object.choice?.$case === "aString" && object.choice?.value !== undefined && object.choice?.value !== null ) { message.choice = { $case: "aString", value: object.choice.value }; } else if ( //... ``` The previous PR I opened adding "else if" does nothing regarding Typescript perf. With "else if", my test file trace looks like this: ``` npx analyze-trace traceDir Hot Spots ├─ Check file /home/dan/projects/ts-proto/integration/large/messages.ts (31108ms) │ ├─ Check deferred node from (line 6267, char 3) to (line 6539, char 4) (13123ms) │ ├─ Check deferred node from (line 15342, char 3) to (line 15591, char 4) (7092ms) │ ├─ Check deferred node from (line 10849, char 3) to (line 11059, char 4) (4460ms) │ ├─ Check deferred node from (line 17320, char 3) to (line 17496, char 4) (2146ms) │ ├─ Check deferred node from (line 3720, char 3) to (line 3832, char 4) (610ms) │ └─ Check deferred node from (line 8791, char 3) to (line 8915, char 4) (516ms) └─ Check file /home/dan/projects/ts-proto/node_modules/typescript/lib/lib.dom.d.ts (1109ms) ``` Now, with this commit, I'm replace `else if` with a `switch` statement. The analysis of the build looks like this: ``` Hot Spots ├─ Check file /home/dan/projects/ts-proto/integration/large/messages.ts (3139ms) │ └─ Check deferred node from (line 6278, char 3) to (line 6573, char 4) (525ms) └─ Check file /home/dan/projects/ts-proto/node_modules/typescript/lib/lib.dom.d.ts (617ms) ``` The biggest `switch` statement takes 525ms to analyze (down from 13seconds with `else if`) Implementation-wise, the hardest part is knowing when to close the switch statement. I'm doing a test at the beginning of each loop and at the very end of the function.
|
Should be fixed in 2.4.2; thanks @moufmouf ! |
Hey!
When migrating from Typescript 5.0 to Typescript 5.1, I noticed a huge performance impact that happens specifically while compiling the files generated by ts-proto.
(the file used to take 6 seconds and suddenly, type checking is taking 110 seconds, which is a complete no-go for us).
I started by opening an issue on the Typescript repository. They closed it (because Typescript 5.1 is apparently doing the right thing and it's normal that it is taking more time).
See: microsoft/TypeScript#60353
More specifically, the Typescript developers gave a look at the generated file and gave a few advices on how to speed up things (namely: replacing multiple "if" with "switch", which would both speed up Typescript compilation and runtime execution).
See: microsoft/TypeScript#60353 (comment)
I thought it might be good to open this issue to show you this feedback.
Also, since the files are auto-generated by ts-proto, doing any type checking on those files is actually useless.
Is there any reason why you don't disable type checking at all by adding a ts-nocheck?
//@ts-nocheck@stephenh I've seen you remove it here #28 (comment) but I don't understand the rationale.
Note: Some other libraries (namely Relay, the GraphQL library) are putting @ts-nocheck by default on generated files and are considering adding a compiler flag to remove it (mostly for internally checking generated files are correctly typed: facebook/relay#4753)
Final note: thanks a lot for ts-proto, it is so much better than the default protobuf JS generation. You guys rock! ❤️
The text was updated successfully, but these errors were encountered: