perf: Generating "else if" where applicable #1141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Still, the "else if" conditions should slightly speed up the runtime execution, by avoiding unnecessary checks to the Javascript runtime.
moufmouf
changed the title
stephenh
approved these changes
Nov 26, 2024
stephenh
pushed a commit
that referenced
this pull request
Nov 26, 2024
## [2.4.1](v2.4.0...v2.4.1) (2024-11-26) ### Performance Improvements * Generating "else if" where applicable ([#1141](#1141)) ([4a8018c](4a8018c))
|
🎉 This issue has been resolved in version 2.4.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
stephenh
pushed a commit
that referenced
this pull request
Nov 28, 2024
…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.
stephenh
pushed a commit
that referenced
this pull request
Nov 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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 😆 )