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

Jump to bottom

child_process: fix deoptimizing use of arguments #11535

Closed
wants to merge 1 commit into from
Closed

child_process: fix deoptimizing use of arguments #11535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
146 changes: 146 additions & 0 deletions benchmark/child_process/child-process-params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
'use strict';

const common = require('../common.js');
const cp = require('child_process');

const command = 'echo';
const args = ['hello'];
const options = {};
const cb = () => {};

const configs = {
n: [1e3],
methodName: [
'exec', 'execSync',
'execFile', 'execFileSync',
'spawn', 'spawnSync',
],
params: [1, 2, 3, 4],
};

const bench = common.createBenchmark(main, configs);

function main(conf) {
const n = +conf.n;
const methodName = conf.methodName;
const params = +conf.params;

const method = cp[methodName];

switch (methodName) {
case 'exec':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command).kill();
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, options).kill();
bench.end(n);
break;
case 3:
bench.start();
for (let i = 0; i < n; i++) method(command, options, cb).kill();
bench.end(n);
break;
}
break;
case 'execSync':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command);
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, options);
bench.end(n);
break;
}
break;
case 'execFile':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command).kill();
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, args).kill();
bench.end(n);
break;
case 3:
bench.start();
for (let i = 0; i < n; i++) method(command, args, options).kill();
bench.end(n);
break;
case 4:
bench.start();
for (let i = 0; i < n; i++) method(command, args, options, cb).kill();
bench.end(n);
break;
}
break;
case 'execFileSync':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command);
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, args);
bench.end(n);
break;
case 3:
bench.start();
for (let i = 0; i < n; i++) method(command, args, options);
bench.end(n);
break;
}
break;
case 'spawn':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command).kill();
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, args).kill();
bench.end(n);
break;
case 3:
bench.start();
for (let i = 0; i < n; i++) method(command, args, options).kill();
bench.end(n);
break;
}
break;
case 'spawnSync':
switch (params) {
case 1:
bench.start();
for (let i = 0; i < n; i++) method(command);
bench.end(n);
break;
case 2:
bench.start();
for (let i = 0; i < n; i++) method(command, args);
bench.end(n);
break;
case 3:
bench.start();
for (let i = 0; i < n; i++) method(command, args, options);
bench.end(n);
break;
}
break;
}
}
32 changes: 12 additions & 20 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,10 @@ exports._forkChild = function(fd) {
};


function normalizeExecArgs(command /*, options, callback*/) {
let options;
let callback;

if (typeof arguments[1] === 'function') {
function normalizeExecArgs(command, options, callback) {
if (typeof options === 'function') {
callback = options;
options = undefined;
callback = arguments[1];
} else {
options = arguments[1];
callback = arguments[2];
}

// Make a shallow copy so we don't clobber the user's options object.
Expand Down Expand Up @@ -156,7 +150,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
callback = arguments[pos++];
}

if (!callback && arguments[pos] != null) {
if (!callback && pos < arguments.length && arguments[pos] != null) {
throw new TypeError('Incorrect value of args option');
}

Expand Down Expand Up @@ -195,6 +189,8 @@ exports.execFile = function(file /*, args, options, callback*/) {

var ex = null;

var cmd = file;

Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It prevents named parameter from leaking in a lower scope causing deopt. See https://bugs.chromium.org/p/v8/issues/detail?id=6010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just upped this line from the inner exithandler() function. It seems this does not cause any scope conflict.

Copy link
Member

Choose a reason for hiding this comment

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

Eh… Understood.
Perhaps this deserves some kind of comment there, /cc @mscdex.

Copy link
Member

Choose a reason for hiding this comment

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

Also /cc @nodejs/v8 and @targos.

I don't think that this is the only place that is affected by our update to V8 5.6, I expect more deopts like this.

This better should be fixed on the V8 side instead of rewriting all the places in Node.js where a named function parameter is used in a child scope.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It happens only if arguments[i] is read, so it could not be a huge number of cases.

Copy link
Contributor

@mscdex mscdex Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the whole arguments[..] and named parameter mixed usage deopt has been around for awhile, long before V8 5.6?

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mscdex These two factors do not cause deopt in:

v8 4.5.103.45 (Node.js 4.8.0 x64)
v8 5.1.281.93 (Node.js 6.10.0 x64)
v8 5.5.372.40 (Node.js 7.6.0 x64)

And I have not seen this case in any articles on v8 optimization.

Copy link
Member

@ChALkeR ChALkeR Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mscdex

I believe the whole arguments[..] and named parameter mixed usage deopt has been around for awhile, long before V8 5.6?

Only outside of the strict mode if I remember things correctly, but I could be mistaken.

The linked bug is about the code in the strict mode.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ChALkeR There was a slightly different case in the sloppy mode: reassigning a defined parameter + mentioning arguments.

function exithandler(code, signal) {
if (exited) return;
exited = true;
Expand Down Expand Up @@ -222,7 +218,6 @@ exports.execFile = function(file /*, args, options, callback*/) {
return;
}

var cmd = file;
if (args.length !== 0)
cmd += ' ' + args.join(' ');

Expand Down Expand Up @@ -330,21 +325,18 @@ function _convertCustomFds(options) {
}
}

function normalizeSpawnArguments(file /*, args, options*/) {
var args, options;

function normalizeSpawnArguments(file, args, options) {
if (typeof file !== 'string' || file.length === 0)
throw new TypeError('"file" argument must be a non-empty string');

if (Array.isArray(arguments[1])) {
args = arguments[1].slice(0);
options = arguments[2];
} else if (arguments[1] !== undefined &&
(arguments[1] === null || typeof arguments[1] !== 'object')) {
if (Array.isArray(args)) {
args = args.slice(0);
} else if (args !== undefined &&
(args === null || typeof args !== 'object')) {
throw new TypeError('Incorrect value of args option');
} else {
options = args;
args = [];
options = arguments[1];
}

if (options === undefined)
Expand Down