Skip to content

util: improve format performance - #15422

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

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

Conversation

@BridgeAR

@BridgeARBridgeAR commented Sep 15, 2017•
edited
Loading

Copy link
Copy Markdown
Member

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-botnodejs-github-bot added the util Issues and PRs related to the built-in util module. label Sep 15, 2017
@BridgeARBridgeAR 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
Comment threadlib/util.js Outdated

@mscdexmscdexSep 15, 2017

Copy link
Copy Markdown
Contributor

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).

@BridgeARBridgeARSep 15, 2017

Copy link
Copy Markdown
MemberAuthor

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

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 15, 2017

Copy link
Copy Markdown
MemberAuthor

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
Comment threadlib/util.js
for (i = 0; i < f.length - 1; i++) {
if (f.charCodeAt(i) === 37) { // '%'
const nextChar = f.charCodeAt(++i);
if (a !== arguments.length) {

@apapirovskiapapirovskiSep 15, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

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?

@apapirovskiapapirovskiSep 15, 2017

Copy link
Copy Markdown
Contributor

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

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
MemberAuthor

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

@BridgeAR

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in faaefa8

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
@BridgeARStephen Belanger
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
@BridgeARStephen Belanger
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>
@MylesBorinsMylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Oct 17, 2017
@MylesBorins

MylesBorins commented Oct 17, 2017

Copy link
Copy Markdown
Contributor

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

Please change labels as appropriate

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

BridgeAR commented Mar 8, 2018

Copy link
Copy Markdown
MemberAuthor

@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

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

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

@BridgeAR

BridgeAR commented Mar 20, 2018

Copy link
Copy Markdown
MemberAuthor

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

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

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
@BridgeARBridgeAR mentioned this pull request Dec 20, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
+3 more reviewers
@apapirovskiapapirovskiapapirovski left review comments
@mscdexmscdexmscdex left review comments
@refackrefackrefack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performanceIssues and PRs related to the performance of Node.js.utilIssues 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