-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
util: improve format performance #24981
Conversation
This simplifies the `format()` code and significantly improves the performance.
@nodejs/performance @nodejs/util PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit.
This simplifies the `format()` code and significantly improves the performance. PR-URL: nodejs#24981 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Landed in 9752fce |
This does not land cleanly on v11.x, would someone be willing to backport? |
I don't think it's worth backporting as the performance regression was mostly introduced by my |
I changed the labels so this is not requested for backporting anymore. |
This simplifies the `format()` code and significantly improves the performance. PR-URL: nodejs#24981 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
This simplifies the
format()
code and significantly improves theperformance.
Just a few runs to verify the gain:
CI https://ci.nodejs.org/job/node-test-pull-request/19442/
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes