Skip to content

util: improve format performance#15422

Closed
BridgeAR wants to merge 2 commits intonodejs:masterfrom
BridgeAR:improve-util-format
Closed

util: improve format performance#15422
BridgeAR wants to merge 2 commits intonodejs:masterfrom
BridgeAR:improve-util-format

Conversation

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 15, 2017 •
edited
Loading

After working on util.inspect I thought it might be worth also checking util.format as that is called by console.log.

 util/format.js type="many-%" n=4000000             19.53 %        *** 5.126437e-12
 util/format.js type="no-replace-2" n=4000000       -1.31 %            4.251080e-01
 util/format.js type="no-replace" n=4000000        105.66 %        *** 4.607428e-46
 util/format.js type="number" n=4000000             17.39 %        *** 3.052166e-20
 util/format.js type="only-objects" n=4000000       17.92 %        *** 6.714987e-15
 util/format.js type="replace-object" n=4000000      8.11 %         ** 1.005488e-03
 util/format.js type="string-2" n=4000000           29.12 %        *** 2.679828e-26
 util/format.js type="string" n=4000000             19.40 %        *** 3.171563e-18
 util/format.js type="unknown" n=4000000           103.28 %        *** 1.826116e-40
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

util

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Sep 15, 2017
@BridgeAR BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Sep 15, 2017
mscdex
mscdex reviewed Sep 15, 2017
View reviewed changes
lib/util.js Outdated
Copy link
Contributor

@mscdex mscdex Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is harder to reason about, which is why I always add extra parentheses so it's always immediately clear (especially to newcomers).

Copy link
Member Author

@BridgeAR BridgeAR Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is interesting to see many people preferring this style. I personally always feel like my brain has to compute a tiny bit more when there are extra braces^^. I changed it back though.

jasnell
jasnell approved these changes Sep 15, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the exception of @mscdex's comment

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 15, 2017

I found a even better way to optimize for unknown types (e.g. if someone uses a percent as a percent).
I updated the code accordingly and also the benchmark results. So PTAL.

apapirovski
apapirovski reviewed Sep 15, 2017
View reviewed changes
lib/util.js
for (i = 0; i < f.length - 1; i++) {
if (f.charCodeAt(i) === 37) { // '%'
const nextChar = f.charCodeAt(++i);
if (a !== arguments.length) {
Copy link
Contributor

@apapirovski apapirovski Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to store arguments.length in a const given the extensive usage throughout format?

Copy link
Contributor

@apapirovski apapirovski Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that might be worse for perf after reviewing how V8 handles it. You can ignore, I believe.

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 19, 2017

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

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 19, 2017

Landed in faaefa8

@BridgeAR BridgeAR closed this Sep 19, 2017
BridgeAR added a commit that referenced this pull request Sep 19, 2017
@BridgeAR
util: improve format performance
faaefa8 
PR-URL: #15422
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
util: improve format performance
fc1448f 
PR-URL: #15422
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
util: improve format performance
be98bf9 
PR-URL: nodejs/node#15422
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
util: improve format performance
a413129 
PR-URL: nodejs/node#15422
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Oct 17, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 17, 2017

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate

@BridgeAR BridgeAR mentioned this pull request Dec 20, 2017
Closed
3 tasks
@BridgeAR BridgeAR removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Mar 8, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

@MylesBorins I think we could still backport this to 8.x but I am not sure if it applies without conflicts or not. Would you be so kind and check that?

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

@BridgeAR this already landed on 8.x, has conflicts on 6.x

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 20, 2018

Oh, right, I missed that. As 6.x is soon in maintenance only, I do not really think that this has to be backported. I changed the label accordingly.

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

thanks @BridgeAR

in future please add the dont-land label so that things don't show up in tooling

@BridgeAR BridgeAR deleted the improve-util-format branch April 1, 2019 23:37
@BridgeAR BridgeAR mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

+3 more reviewers

@apapirovski apapirovski apapirovski left review comments

@mscdex mscdex mscdex left review comments

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@BridgeAR @MylesBorins @apapirovski @mscdex @refack @jasnell @nodejs-github-bot