-
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 util.format performance #5360
Conversation
@@ -21,21 +29,20 @@ exports.format = function(f) { | |||
|
|||
if (arguments.length === 1) return f; | |||
|
|||
const len = arguments.length; | |||
const args = new Array(len); | |||
for (let i = 0; i < len; i++) { |
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.
Use var
here until v8 can optimize let
better.
447e7f1
to
88039b9
Compare
@@ -21,21 +29,20 @@ exports.format = function(f) { | |||
|
|||
if (arguments.length === 1) return f; | |||
|
|||
const len = arguments.length; | |||
const args = new Array(len); | |||
for (var i = 0; i < len; i++) { |
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.
Sorry, you'll also need to move the var i
declaration out and above and change var i = 1;
a few lines below to i = 1
to avoid a linter error.
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.
yep, just noticed that locally. Thanks
88039b9
to
429ad25
Compare
For bonus points, you can change for (var x = args[i]; i < len; x = args[++i]) { to while (i < len) {
const x = args[i++]; to avoid an eager deopt due to potential out of bounds access on |
7a00928
to
91fc8b4
Compare
Ah, totally missed the case where |
args[i] = arguments[i]; | ||
} | ||
|
||
i = 1; | ||
var str = String(f).replace(formatRegExp, function(x) { |
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.
String(f)
here can probably just be replaced with just f
since it should already be a string at that point.
ooo good catch |
91fc8b4
to
6fd10fe
Compare
Just for fun I replaced the regexp + replace function with a simple loop and improved performance an additional 60-200% in the benchmarks included (and no change in the |
wow, nice |
Marking this LTS watch but I'd rather this sit for a bit before getting pulled back. |
LGTM |
774a86e
to
fe2996a
Compare
Running CI again: https://ci.nodejs.org/job/node-test-pull-request/1851/ |
PR-URL: nodejs#5360 Reviewed-By: James M Snell <jasnell@gmail.com>
By manually copying arguments and breaking the try/catch out, we are able to improve the performance of util.format by 20-100% (depending on the types). PR-URL: nodejs#5360 Reviewed-By: James M Snell <jasnell@gmail.com>
Replacing the regexp and replace function with a loop improves performance by ~60-200%. PR-URL: nodejs#5360 Reviewed-By: James M Snell <jasnell@gmail.com>
fe2996a
to
c490b8b
Compare
@jasnell would you want to include this in a future lts? |
I believe so yes. |
@jasnell / @nodejs/lts how much longer to we want this to live before backporting? |
I haven't heard of any regressions. We may be good on this one
|
By manually copying arguments and breaking the try/catch out, we are able to improve the performance of util.format by 20-100% (depending on the types). PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
Replacing the regexp and replace function with a loop improves performance by ~60-200%. PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
I've gone ahead and added this to v4.x-staging and will include it in the v4.5.0 rc. @evanlucas please let me know if there are any accompanying patch's we need to make this work |
@evanlucas as a heads up you need to provide the commit before your initial commit when doing the 735e0df...c490b8b does not include 735e0df |
By manually copying arguments and breaking the try/catch out, we are able to improve the performance of util.format by 20-100% (depending on the types). PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
Replacing the regexp and replace function with a loop improves performance by ~60-200%. PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
@thealphanerd there shouldn't be anything else need to land these. Thanks for the heads up on the range syntax as well :] |
By manually copying arguments and breaking the try/catch out, we are able to improve the performance of util.format by 20-100% (depending on the types). PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
Replacing the regexp and replace function with a loop improves performance by ~60-200%. PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #5360 Reviewed-By: James M Snell <jasnell@gmail.com>
By manually copying arguments and breaking the try/catch out, we are able to improve the performance of util.format by 20-100% (depending on the types).
Also includes a util.format benchmark.
The numbers:
EDIT: I updated the numbers after rebasing on master and with @mscdex's changes included as well.