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: escaping object keys in util.inspect() #16986

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

ah-yu
Copy link
Contributor

@ah-yu ah-yu commented Nov 13, 2017

Fixes: #16979

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

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

refack commented Nov 13, 2017

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


assert.strictEqual(
util.inspect(w),
'{ \'\\\': 1, \'\\\\\': 2, \'\\\\\\\': 3, \'\\\\\\\\\': 4 }'
'{ \'\\\\\': 2, \'\\\\\\\\\': 4, \'\\\\\\\\\\\\\': 6, ' +
'\'\\\\\\\\\\\\\\\\\': 8 }'
);
assert.strictEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your suggestion, I will add it later

@refack refack self-assigned this Nov 13, 2017
@vsemozhetbyt
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Nov 13, 2017

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

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

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link

Choose a reason for hiding this comment

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

Could you open a bug at crbug.com/v8/new?

Went ahead and created https://crbug.com/v8/7081.

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.

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
Copy link
Contributor Author

ah-yu commented Nov 14, 2017

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

@bmeurer
Copy link
Member

bmeurer commented Nov 14, 2017

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

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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

@apapirovski
Copy link
Member

@addaleax
Copy link
Member

Landed in 71ee0d9, thanks for the PR! ✨

@addaleax addaleax closed this Nov 18, 2017
addaleax pushed a commit that referenced this pull request Nov 18, 2017
PR-URL: #16986
Fixes: #16979
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@refack refack removed their assignment Nov 18, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16986
Fixes: #16979
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 19, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Dec 19, 2017

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

@BridgeAR 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
Copy link
Member

BridgeAR commented Mar 8, 2018

@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
Copy link
Contributor

MylesBorins commented Mar 20, 2018

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

edit: I've landed in"

@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
PR-URL: #16986
Fixes: #16979
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
PR-URL: #16986
Fixes: #16979
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
PR-URL: #16986
Fixes: #16979
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util: escaping object keys in util.inspect()