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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
369f64a
util: escaping object keys in util.inspect()
ah-yu Nov 13, 2017
a61e8df
test：add new test cases in test-util-inspect.js
ah-yu Nov 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 1 addition & 31 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,7 @@ var Debug;

/* eslint-disable */
const strEscapeSequencesRegExp = /[\x00-\x1f\x27\x5c]/;
const keyEscapeSequencesRegExp = /[\x00-\x1f\x27]/;
const strEscapeSequencesReplacer = /[\x00-\x1f\x27\x5c]/g;
const keyEscapeSequencesReplacer = /[\x00-\x1f\x27]/g;
/* eslint-enable */
const keyStrRegExp = /^[a-zA-Z_][a-zA-Z_0-9]*$/;
const colorRegExp = /\u001b\[\d\d?m/g;
Expand Down Expand Up @@ -137,34 +135,6 @@ function strEscape(str) {
return `'${result}'`;
}

// Escape control characters and single quotes.
// Note: for performance reasons this is not combined with strEscape
function keyEscape(str) {
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.

var result = '';
var last = 0;
for (var i = 0; i < str.length; i++) {
const point = str.charCodeAt(i);
if (point === 39 || point < 32) {
if (last === i) {
result += meta[point];
} else {
result += `${str.slice(last, i)}${meta[point]}`;
}
last = i + 1;
}
}
if (last === 0) {
result = str;
} else if (last !== i) {
result += str.slice(last);
}
return `'${result}'`;
}

function tryStringify(arg) {
try {
return JSON.stringify(arg);
Expand Down Expand Up @@ -859,7 +829,7 @@ function formatProperty(ctx, value, recurseTimes, key, array) {
} else if (keyStrRegExp.test(key)) {
name = ctx.stylize(key, 'name');
} else {
name = ctx.stylize(keyEscape(key), 'string');
name = ctx.stylize(strEscape(key), 'string');
}

return `${name}: ${str}`;
Expand Down
15 changes: 8 additions & 7 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,22 +570,23 @@ assert.doesNotThrow(() => {
// util.inspect should not display the escaped value of a key.
{
const w = {
'\\': 1,
'\\\\': 2,
'\\\\\\': 3,
'\\\\\\\\': 4,
'\\': 2,
'\\\\': 4,
'\\\\\\': 6,
'\\\\\\\\': 8,
};

const y = ['a', 'b', 'c'];
y['\\\\\\'] = 'd';
y['\\\\'] = 'd';

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

util.inspect(y),
'[ \'a\', \'b\', \'c\', \'\\\\\\\': \'d\' ]'
'[ \'a\', \'b\', \'c\', \'\\\\\\\\\': \'d\' ]'
);
}

Expand Down