-
Notifications
You must be signed in to change notification settings - Fork 437
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
feat: improve performance on Node.js 16 by disabling class property definitions #1581
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1581 +/- ##
=======================================
Coverage 79.78% 79.78%
=======================================
Files 92 92
Lines 4709 4709
Branches 871 871
=======================================
Hits 3757 3757
Misses 684 684
Partials 268 268
|
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier has some more information around this, and the |
@MichaelSun90 Do you have the time to run a few of the benchmarks before / after this change and document the difference here in this pull request body? 🙇♂️ |
Here are the benchmark compression ( Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
Without the change:
With the change:
|
@MichaelSun90 I assume the Node.js version you used was v16? The results are a bit all over the place, I'd have expected them to be a bit more consistent in the improvement. 🤔 Obviously, the token parsing ones should be the one that show the biggest difference, because that's the part that's most affected by the performance degradation coming from using class properties. The benchmarks that test full request cycles are expectedly less affected, because token parsing is a small piece of the overall logic that gets executed in these benchmarks. The bulk-load benchmarks and the request payload generation benchmark should also only show minimal changes, but there's quite some variation in there. If you have a lot of other processes running on your machines, that might make the benchmarks less accurate. |
I am on node 18.17.1. 🤔 The improvement on the token parsing part looks pretty promising. |
🎉 This PR is included in version 16.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
add declare for class level properties regard to #1579