util: escaping object keys in util.inspect()#16986

ah-yu · 2017-11-13T11:56:36Z

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)

lib

refack · 2017-11-13T12:05:35Z

Hello @buji1993 and welcome. Thank you very much for the contribution 🥇
If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especially the part about "discuss and update"). Customarily PRs are kept open for at least 48 hour so that anyone interested gets a chance to comment or review.

P.S. If you have any question you can also feel free to contact me directly (here, on IRC, or anywhere else).

refack · 2017-11-13T12:09:12Z

test/parallel/test-util-inspect.js

+'{\'\\\\\': 2, \'\\\\\\\\\': 4, \'\\\\\\\\\\\\\': 6, '+
+'\'\\\\\\\\\\\\\\\\\': 8 }'
 );
 assert.strictEqual(


Suggestion, could you add a test with \r and \n

thanks for your suggestion, I will add it later

vsemozhetbyt · 2017-11-13T18:42:40Z

Is this semver-major? It seems like a bugfix, but it changes outputs.

jasnell · 2017-11-13T19:29:23Z

I'd argue for bug fix, but once we land, we might want to wait a while before backporting to LTS

BridgeAR · 2017-11-14T03:20:40Z

lib/util.js

-if(str.length<5000&&!keyEscapeSequencesRegExp.test(str))
-return`'${str}'`;
-if(str.length>100)
-return`'${str.replace(keyEscapeSequencesReplacer,escapeFn)}'`;


Not specific to this PR but to the code:
@bmeurer in my tests RegExp.p.test performs better than String.p.replace. Should those not be on par in case no match is found? And with small strings it is (last tested with 6.1) still better to use String.p.charCodeAt instead of the RegExp. Is there any chance to improve the RegExp so it would match the performance of charCodeAt?

@bmeurer in my tests RegExp.p.test performs better than String.p.replace. Should those not be on par in case no match is found?
test is a much simpler operation than @@replace (see the spec at [0] and [1]). It's fairly easy to implement test efficiently; but in my experience @@replace is another story, see V8's @@replace dispatch logic at [2] to feel our pain.
In this particular case, we seem to reach the ReplaceGlobalCallableFastPath. There's definitely a couple of things we could do to improve here, like remove one (or both) runtime calls. Could you open a bug at crbug.com/v8/new?
[0] https://tc39.github.io/ecma262/#sec-regexp.prototype.test
[1] https://tc39.github.io/ecma262/#sec-regexp.prototype-@@replace
[2] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-regexp-gen.cc?l=3058&rcl=2eea37273b30caa5cedf3a5c8d656860bc60320b
And with small strings it is (last tested with 6.1) still better to use String.p.charCodeAt instead of the RegExp. Is there any chance to improve the RegExp so it would match the performance of charCodeAt?
I assume you mean charCodeAt vs. test? It'll be hard to beat, since charCodeAt is completely inlined by the optimizing compiler. I suppose one possible step we could take would be to eliminate the call overhead from RegExp builtins, but I wouldn't expect performance to improve by alot.
Could you share the benchmark you used to measure this?

Could you open a bug at crbug.com/v8/new?
Went ahead and created https://crbug.com/v8/7081.

BridgeAR

LGTM

Seems like the original reason for this was another bug (nodejs/node-v0.x-archive#6835) that was fixed with a not ideal solution.

ah-yu · 2017-11-14T05:54:16Z

@refack thanks, it's so nice! I will contact you if I need help

bmeurer · 2017-11-14T06:35:36Z

@schuay is probably a good person to answer this question.

apapirovski

PRs that fix a bug by removing code are my favourite. 🥇

apapirovski · 2017-11-14T18:18:53Z

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

addaleax · 2017-11-18T19:53:08Z

Landed in 71ee0d9, thanks for the PR! ✨

PR-URL: #16986Fixes: #16979 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

MylesBorins · 2017-12-19T20:24:54Z

as this may have unexpected breakages we are waiting a bit before landing on lts

BridgeAR · 2018-03-08T14:53:29Z

@MylesBorins we now got this in 9 for a while without any complains I think we can go ahead and backport this. I added the backport requested label but it might actually apply cleanly?

MylesBorins · 2018-03-20T14:54:35Z

@BridgeAR the land-on label is used once something has been landed. 😄

edit: I've landed in"

MylesBorins · 2018-03-20T14:55:17Z

I've opted to not land on v6.x. Please feel free to change labels and open backport

PR-URL: #16986Fixes: #16979 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

util: escaping object keys in util.inspect()
369f64a
Fixes: #16979

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 13, 2017

refack reviewed Nov 13, 2017
View reviewed changes

refack self-assigned this Nov 13, 2017

addaleax approved these changes Nov 13, 2017
View reviewed changes

jasnell approved these changes Nov 13, 2017
View reviewed changes

BridgeAR reviewed Nov 14, 2017
View reviewed changes

BridgeAR approved these changes Nov 14, 2017
View reviewed changes

test：add new test cases in test-util-inspect.js
a61e8df

apapirovski approved these changes Nov 14, 2017
View reviewed changes

addaleax closed this Nov 18, 2017

refack removed their assignment Nov 18, 2017

MylesBorins mentioned this pull request Dec 12, 2017
v9.3.0 proposal #17631
Merged

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 19, 2017

BridgeAR added backport-requested-v8.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Mar 8, 2018

BridgeAR removed the lts-watch-v8.x label Mar 8, 2018

BridgeAR added land-on-v8.x and removed backport-requested-v8.x labels Mar 8, 2018

MylesBorins added the dont-land-on-v6.x label Mar 20, 2018

MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged

schuay mentioned this pull request Apr 13, 2021
🚀 Rework unesc for a 63+% performance boost to all of postcss. postcss/postcss-selector-parser#239
Merged

Uh oh!

util: escaping object keys in util.inspect()#16986

util: escaping object keys in util.inspect() #16986

Uh oh!

Conversation

ah-yu commented Nov 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Nov 13, 2017

Uh oh!

refackNov 13, 2017

Choose a reason for hiding this comment

Uh oh!

ah-yuNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Nov 13, 2017

Uh oh!

jasnell commented Nov 13, 2017

Uh oh!

BridgeARNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

schuayNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

schuayNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

ah-yu commented Nov 14, 2017

Uh oh!

bmeurer commented Nov 14, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Nov 14, 2017

Uh oh!

addaleax commented Nov 18, 2017

Uh oh!

MylesBorins commented Dec 19, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

MylesBorins commented Mar 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

MylesBorins commented Dec 19, 2017•
edited
Loading

MylesBorins commented Mar 20, 2018•
edited
Loading