util: improve util.format performance

[evanlucas]

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:

running ./node util/format.js running ./node_before util/format.js util/format.js n=1000000 type=string: ./node: 7866600 ./node_before: 1705900 ... 361.13% util/format.js n=1000000 type=number: ./node: 7718000 ./node_before: 1778900 ... 333.87% util/format.js n=1000000 type=object: ./node: 1811800 ./node_before: 800160 .... 126.43% util/format.js n=1000000 type=unknown: ./node: 13045000 ./node_before: 2650300 . 392.22% util/format.js n=1000000 type=no-replace: ./node: 1541500 ./node_before: 956760 . 61.11% 

EDIT: I updated the numbers after rebasing on master and with @mscdex's changes included as well.

[evanlucas]

CI: https://ci.nodejs.org/job/node-test-pull-request/1716/

[mscdex]

Use var here until v8 can optimize let better.

[mscdex]

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.

[evanlucas]

yep, just noticed that locally. Thanks

[mscdex]

For bonus points, you can change

for(varx=args[i];i<len;x=args[++i]){

to

while(i<len){constx=args[i++];

to avoid an eager deopt due to potential out of bounds access on args.

[evanlucas]

Ah, totally missed the case where f is not a string. Added a benchmark and updated for that as well

[mscdex]

String(f) here can probably just be replaced with just f since it should already be a string at that point.

[evanlucas]

ooo good catch

[mscdex]

CI again: https://ci.nodejs.org/job/node-test-pull-request/1718/

[mscdex]

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 no-replace case) in this PR.

[evanlucas]

wow, nice

[jasnell]

Marking this LTS watch but I'd rather this sit for a bit before getting pulled back.

[jasnell]

LGTM

[evanlucas]

Running CI again: https://ci.nodejs.org/job/node-test-pull-request/1851/

[MylesBorins]

@jasnell would you want to include this in a future lts?

[jasnell]

I believe so yes.

[MylesBorins]

@jasnell / @nodejs/lts how much longer to we want this to live before backporting?

[jasnell]

I haven't heard of any regressions. We may be good on this one
On Jun 30, 2016 5:11 PM, "Myles Borins" [email protected] wrote:

@jasnellhttps://github.com/jasnell / @nodejs/lts
https://github.com/orgs/nodejs/teams/lts how much longer to we want
this to live before backporting?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5360 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eVV5PJs-GWjQjX7nl55qNJMiSNYGks5qRFtAgaJpZM4HfWa3
.

[MylesBorins]

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

[MylesBorins]

@evanlucas as a heads up you need to provide the commit before your initial commit when doing the ... syntax to specify a range.

735e0df...c490b8b does not include 735e0df

[evanlucas]

@thealphanerd there shouldn't be anything else need to land these. Thanks for the heads up on the range syntax as well :]