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

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

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process, benchmarks

This is the last unaddressed case from mentioned in the #10323. PR fixes three functions.

1. Function normalizeExecArgs() is called by exports.exec() and exports.execSync() with a variable number of parameters.

To check deoptimizations, run this code with --trace_opt --trace_deopt flags:

const execSync = require('child_process').execSync;
for (let i = 0; i < 1e3; i++) execSync('echo');

You will see messages about normalizeExecArgs() deoptimization.

None of the current benchmarks checks this case: the only benchmark calling exports.exec() does it once and always with 2 parameters.

2. Function exports.execFile() is called by users with a variable number of parameters. See also #10323 (comment)

To check deoptimizations, run this code with --trace_opt --trace_deopt flags:

const execFile = require('child_process').execFile;
for (let i = 0; i < 1e3; i++) execFile('echo').kill();

You will see messages about exports.execFile() deoptimization.

None of the current benchmarks checks exports.execFile().

3. Function normalizeSpawnArguments() is called by exports.spawn(), exports.spawnSync(), and exports.execFileSync() with a variable number of parameters.

To check deoptimizations, run this code with --trace_opt --trace_deopt flags:

const spawnSync = require('child_process').spawnSync;
for (let i = 0; i < 1e3; i++) spawnSync('echo');

You will see messages about normalizeSpawnArguments() deoptimization.

None of the current benchmarks checks exports.spawnSync() or exports.execFileSync(). Two of them call exports.spawn() once with 3 params (not appropriate). One of them calls exports.spawn() 1000 times with 2 params, but it does not cover all sub-cases.

4. While fixing these cases, I've found an absolutely new deopt case in the exports.execFile() function introduced by v8 5.6. See the fourth commit and explanation in the upstream issue.

5. As changes affect almost all main child_process methods (except .fork()), I've added a benchmark calling these 6 methods with 1, 2, 3, and 4 params (when it is appropriate). Benchmarking is a bit murky in this case because of many external sync/async processes involved, but it seems no significant degradation is caused, moreover some tiny improvement is evident.

                                                                                  improvement confidence      p.value
 child_process\\child-process-params.js params=1 methodName="exec" n=1000              0.59 %            6.187539e-02
 child_process\\child-process-params.js params=1 methodName="execFile" n=1000          0.62 %          * 2.916666e-02
 child_process\\child-process-params.js params=1 methodName="execFileSync" n=1000      0.32 %            5.213650e-01
 child_process\\child-process-params.js params=1 methodName="execSync" n=1000          0.26 %          * 4.716863e-02
 child_process\\child-process-params.js params=1 methodName="spawn" n=1000             0.69 %        *** 2.204024e-04
 child_process\\child-process-params.js params=1 methodName="spawnSync" n=1000         0.19 %            7.165778e-01
 child_process\\child-process-params.js params=2 methodName="exec" n=1000              0.72 %          * 4.544538e-02
 child_process\\child-process-params.js params=2 methodName="execFile" n=1000          0.68 %          * 2.451089e-02
 child_process\\child-process-params.js params=2 methodName="execFileSync" n=1000     -0.30 %            5.470930e-01
 child_process\\child-process-params.js params=2 methodName="execSync" n=1000          0.30 %            1.907476e-01
 child_process\\child-process-params.js params=2 methodName="spawn" n=1000             0.40 %         ** 6.548929e-03
 child_process\\child-process-params.js params=2 methodName="spawnSync" n=1000         0.51 %            2.825198e-01
 child_process\\child-process-params.js params=3 methodName="exec" n=1000              0.37 %            2.182035e-01
 child_process\\child-process-params.js params=3 methodName="execFile" n=1000          0.86 %        *** 1.298733e-06
 child_process\\child-process-params.js params=3 methodName="execFileSync" n=1000      0.22 %            7.333930e-01
 child_process\\child-process-params.js params=3 methodName="spawn" n=1000             0.95 %         ** 7.015692e-03
 child_process\\child-process-params.js params=3 methodName="spawnSync" n=1000        -0.43 %            4.574352e-01
 child_process\\child-process-params.js params=4 methodName="execFile" n=1000          1.15 %        *** 6.094645e-07

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Feb 24, 2017
@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Feb 24, 2017
@@ -195,6 +195,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

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

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

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

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

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.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 24, 2017

This seems like the wrong approach to me. It introduces more uses of arguments instead of fewer. Could the changes to normalizeExecArgs() and normalizeSpawnArgs() be made by adding named function parameters instead (I think you just made similar changes in dgram)?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 24, 2017

@cjihrig I was not sure I should refactor it, I've just tried not to be very intrusive. But yes, I can try to add named parameters.

@vsemozhetbyt
Copy link
Contributor Author

@cjihrig Fixed for normalizeExecArgs() and normalizeSpawnArgs(). I've tried to copy the approach in the dgram.

@vsemozhetbyt
Copy link
Contributor Author

Build and tests are OK. I will launch a new benchmark run and will post the results a bit later.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

This looks better to me. One last comment though. Is there a performance hit for reassigning named parameters. It looks like we might be able to get away with not having separate variables from the parameters. I know we do that in other places in core (and it looks cleaner), but again, I'm not sure about any performance hit.

let options;
let callback;

if (typeof arguments[1] === 'function') {
if (options_ && typeof options_ === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need the leading options_ now, right?

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 though it would eliminate unneeded type check / function call (isArray() below) on undefined and would be a bit faster. Should I delete them?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to know what V8 will do from version to version, but won't the leading options_ require a conversion to a boolean and comparison against true?

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 seems so) So should I delete them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

options = arguments[2];
} else if (arguments[1] !== undefined &&
(arguments[1] === null || typeof arguments[1] !== 'object')) {
if (args_ && Array.isArray(args_)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about the leading args_

@vsemozhetbyt
Copy link
Contributor Author

@cjihrig If I remember right, in the ES6 named parameters and arguments are disconnected, so the redefining of named parameters has less performance impact. However, there is still a popular style rule for this case.

Maybe this simplification could be done as a separate PR by a more experienced contributor? It seems there are many other similar cases in the libs.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 24, 2017

However, there is still a popular style rule for this case.

I don't think that matters. That's more of a style preference. Since we already do it in core, I think it only matters if it hurts performance.

Maybe this simplification could be done as a separate PR by a more experienced contributor?

I'd rather get this figured out in one PR than having a second PR to change the exact same thing (churn, backporting overhead, etc.).

@vsemozhetbyt
Copy link
Contributor Author

So should I wait till some more opinions or should I remove the mediatorial params now?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 24, 2017

@mscdex probably knows the answer :-)


const method = cp[methodName];

if (methodName === 'exec') {
Copy link
Member

Choose a reason for hiding this comment

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

This would be more manageable as a switch (methodName) {} structure... with each case handled in a separate function.

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 will try.

const method = cp[methodName];

if (methodName === 'exec') {
if (params === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

This would also be better as a switch (params) {} block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully fixed.

@vsemozhetbyt
Copy link
Contributor Author

New benchmark with amended commits:

                                                                                  improvement confidence      p.value
 child_process\\child-process-params.js params=1 methodName="exec" n=1000              0.81 %        *** 0.0003641932
 child_process\\child-process-params.js params=1 methodName="execFile" n=1000          0.69 %          * 0.0463235196
 child_process\\child-process-params.js params=1 methodName="execFileSync" n=1000     -0.15 %            0.8497728229
 child_process\\child-process-params.js params=1 methodName="execSync" n=1000          0.02 %            0.8812585510
 child_process\\child-process-params.js params=1 methodName="spawn" n=1000             0.49 %            0.0689109073
 child_process\\child-process-params.js params=1 methodName="spawnSync" n=1000        -0.75 %            0.3482902808
 child_process\\child-process-params.js params=2 methodName="exec" n=1000              0.26 %            0.2333704471
 child_process\\child-process-params.js params=2 methodName="execFile" n=1000          0.84 %          * 0.0352815086
 child_process\\child-process-params.js params=2 methodName="execFileSync" n=1000     -0.51 %            0.4581887918
 child_process\\child-process-params.js params=2 methodName="execSync" n=1000          0.10 %            0.3326917806
 child_process\\child-process-params.js params=2 methodName="spawn" n=1000             0.47 %            0.1045985310
 child_process\\child-process-params.js params=2 methodName="spawnSync" n=1000         0.25 %            0.7458324210
 child_process\\child-process-params.js params=3 methodName="exec" n=1000              0.41 %            0.0730991610
 child_process\\child-process-params.js params=3 methodName="execFile" n=1000          0.70 %            0.1057560550
 child_process\\child-process-params.js params=3 methodName="execFileSync" n=1000      0.92 %            0.1793174691
 child_process\\child-process-params.js params=3 methodName="spawn" n=1000             0.79 %         ** 0.0079657734
 child_process\\child-process-params.js params=3 methodName="spawnSync" n=1000         0.27 %            0.6358194310
 child_process\\child-process-params.js params=4 methodName="execFile" n=1000          0.78 %            0.1491293323

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 28, 2017

@cjihrig, @jasnell How should I proceed?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 28, 2017

@vsemozhetbyt would you be OK with using your benchmark to investigate the idea from #11535 (review). Then, we can get this through the CI and landed.

@vsemozhetbyt
Copy link
Contributor Author

@cjihrig It seems OK:

                                                                                  improvement confidence      p.value
 child_process\\child-process-params.js params=1 methodName="exec" n=1000              0.58 %            1.169842e-01
 child_process\\child-process-params.js params=1 methodName="execFile" n=1000          0.74 %          * 1.589342e-02
 child_process\\child-process-params.js params=1 methodName="execFileSync" n=1000      1.04 %            1.439200e-01
 child_process\\child-process-params.js params=1 methodName="execSync" n=1000          0.30 %          * 3.973428e-02
 child_process\\child-process-params.js params=1 methodName="spawn" n=1000             0.23 %            2.825498e-01
 child_process\\child-process-params.js params=1 methodName="spawnSync" n=1000         0.86 %            3.056963e-01
 child_process\\child-process-params.js params=2 methodName="exec" n=1000              0.79 %         ** 3.069827e-03
 child_process\\child-process-params.js params=2 methodName="execFile" n=1000          1.25 %        *** 4.789426e-06
 child_process\\child-process-params.js params=2 methodName="execFileSync" n=1000      0.04 %            9.563199e-01
 child_process\\child-process-params.js params=2 methodName="execSync" n=1000          0.14 %            1.048517e-01
 child_process\\child-process-params.js params=2 methodName="spawn" n=1000             0.13 %            7.351656e-01
 child_process\\child-process-params.js params=2 methodName="spawnSync" n=1000         0.18 %            8.267698e-01
 child_process\\child-process-params.js params=3 methodName="exec" n=1000              0.55 %          * 4.067630e-02
 child_process\\child-process-params.js params=3 methodName="execFile" n=1000          1.27 %        *** 1.786565e-06
 child_process\\child-process-params.js params=3 methodName="execFileSync" n=1000      0.24 %            6.705638e-01
 child_process\\child-process-params.js params=3 methodName="spawn" n=1000             0.07 %            7.901901e-01
 child_process\\child-process-params.js params=3 methodName="spawnSync" n=1000        -0.61 %            3.780685e-01
 child_process\\child-process-params.js params=4 methodName="execFile" n=1000          1.30 %        *** 3.215549e-07

@cjihrig
Copy link
Contributor

cjihrig commented Mar 1, 2017

Awesome. The code looks cleaner, and has less use of arguments. LGTM. Can you squash it please?

Fixed functions: normalizeExecArgs(),
exports.execFile(), normalizeSpawnArguments()

Refs: #10323
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=6010
@vsemozhetbyt
Copy link
Contributor Author

Squashed.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 1, 2017

@cjihrig
Copy link
Contributor

cjihrig commented Mar 1, 2017

Landed in dd81d53. Thanks for working through this.

@cjihrig cjihrig closed this Mar 1, 2017
cjihrig pushed a commit that referenced this pull request Mar 1, 2017
Removed or fixed use of arguments in execFile(),
normalizeExecArgs(), and normalizeSpawnArguments().

Refs: #10323
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=6010
PR-URL: #11535
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@vsemozhetbyt vsemozhetbyt deleted the fix-child-process-arguments-1 branch March 1, 2017 15:38
@evanlucas
Copy link
Contributor

This is not landing cleanly on v7.x-staging. Mind backporting?

@vsemozhetbyt
Copy link
Contributor Author

@evanlucas I never backported a PR. I shall try to learn how to do this and then shall try to apply if nobody does it sooner.

@vsemozhetbyt
Copy link
Contributor Author

@evanlucas PTAL: #11748

jasnell pushed a commit that referenced this pull request Mar 16, 2017
Removed or fixed use of arguments in execFile(),
normalizeExecArgs(), and normalizeSpawnArguments().

Refs: #10323
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=6010

Backport-Of: #11535
PR-URL: #11748
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

See #11748 (comment)

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@vsemozhetbyt
Copy link
Contributor Author

@gibfahn Hopefully done: #13752

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Remove use of arguments in
normalizeExecArgs() and normalizeSpawnArguments().

Refs: #10323
PR-URL: #11535
Backport-PR-URL: #13752
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Remove use of arguments in
normalizeExecArgs() and normalizeSpawnArguments().

Refs: #10323
PR-URL: #11535
Backport-PR-URL: #13752
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants