Skip to content
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: use string width in util formatPrimitive() string length line check #43721

Conversation

Anemy
Copy link
Contributor

@Anemy Anemy commented Jul 7, 2022

Improves support for util.inspect formatting strings with unicode characters by updating to look at the string width instead of the string length.

Characters like have a width of 2 and length 1.

Improves support for formatting strings with unicode characters by
using the string character width instead of the string length.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jul 7, 2022
@targos targos added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the needs-benchmark-ci PR that need a benchmark CI run. label Jul 8, 2022
@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

I would like to run a benchmark on this to make sure the performance hit is not too big.

Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1162/

@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

Let's wait until our benchmarks are fixed.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 8, 2022
@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

This does slow down printing strings significantly (only 2 stars and more):


17:14:46 util/format.js type='only-objects' n=100000                                                            **     -3.85 %       ±2.31% ±3.07%  ±3.98%
17:14:46 util/inspect-array.js type='denseArray' len=100 n=500                                                  **     -6.02 %       ±3.97% ±5.27%  ±6.81%
17:14:46 util/inspect-array.js type='denseArray_showHidden' len=100 n=500                                      ***     -8.34 %       ±4.14% ±5.50%  ±7.13%
17:14:46 util/inspect.js option='colors' method='Object_deep_ln' n=20000                                       ***     -5.35 %       ±1.80% ±2.39%  ±3.09%
17:14:46 util/inspect.js option='colors' method='Object' n=20000                                               ***     -5.80 %       ±2.90% ±3.85%  ±4.98%
17:14:46 util/inspect.js option='colors' method='String_boxed' n=20000                                          **     -6.64 %       ±4.01% ±5.32%  ±6.88%
17:14:46 util/inspect.js option='colors' method='String_complex' n=20000                                       ***    -14.84 %       ±3.98% ±5.27%  ±6.82%
17:14:46 util/inspect.js option='colors' method='String' n=20000                                               ***    -19.74 %       ±5.09% ±6.75%  ±8.73%
17:14:46 util/inspect.js option='none' method='Object_deep_ln' n=20000                                          **     -3.24 %       ±2.03% ±2.69%  ±3.48%
17:14:46 util/inspect.js option='none' method='String_complex' n=20000                                         ***    -12.09 %       ±4.06% ±5.38%  ±6.96%
17:14:46 util/inspect.js option='none' method='String' n=20000                                                 ***    -14.73 %       ±4.50% ±5.97%  ±7.72%
17:14:46 util/inspect.js option='showHidden' method='Error' n=20000                                            ***     -5.24 %       ±2.33% ±3.09%  ±4.01%
17:14:46 util/inspect.js option='showHidden' method='String_complex' n=20000                                    **     -6.10 %       ±4.30% ±5.69%  ±7.36%
17:14:46 util/inspect.js option='showHidden' method='String' n=20000                                           ***    -11.18 %       ±3.65% ±4.84%  ±6.26%

I wonder if we really want this by default. We do not profit from it strongly as it is only going to be used for formatting things nicer under very special circumstances. I already feared the performance drop when I wrote the comment and did not implement it at that point due to that.
By now, I believe it is be best to just accept some input to be formatted slightly different to have a better average performance.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I suggest to just remove the comment instead to prevent the slowdown in the average case.

@Anemy
Copy link
Contributor Author

Anemy commented Jul 10, 2022

@BridgeAR Thanks for running the benchmarks! Totally agree, removed the TODO in a separate pr: #43762
Closing this, I can alternatively reopen and do the change in this pr if that's better.

@Anemy Anemy closed this Jul 10, 2022
@Anemy Anemy deleted the util-update-format-string-length-check-using-width branch July 10, 2022 16:14
aduh95 pushed a commit that referenced this pull request Jul 24, 2022
PR-URL: #43762
Refs: #43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
danielleadams pushed a commit that referenced this pull request Jul 26, 2022
PR-URL: #43762
Refs: #43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43762
Refs: #43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43762
Refs: #43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Aug 1, 2022
PR-URL: #43762
Refs: #43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#43762
Refs: nodejs#43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43762
Refs: nodejs/node#43721
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants