(v6.x backport) child_process: fix deoptimizing use of arguments

[vsemozhetbyt]

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)

benchmark, child_process

Backport of #11535

I've removed changes in execFile(): one cause was not in code yet, another one is relevant only from v8 5.6.

Commit message also was edited: one Refs was removed, the wording was a bit corrected. All other metadata was left untouched.

I've checked the benchmark with the last v6. We need to run CI for the lib.

[gibfahn]

It would be good to get a review or two from @nodejs/performance, @targos, @cjihrig, @jasnell (who reviewed the original PR).

I've checked the benchmark with the last v6. We need to run CI for the lib.

So you still see the perf benefits on v6.x? Could you post some results?

[gibfahn]

CI: https://ci.nodejs.org/job/node-test-commit/10636/

[vsemozhetbyt]

@gibfahn I've just roughly checked if it is compatible and can be run on v6.
Should I build two v6 and run it with a comparison?

[mscdex]

@vsemozhetbyt Yes, before and after this PR.

[vsemozhetbyt]

@gibfahn @mscdex It seems we still have a tiny certain improvement in two cases without any significant downgrade in other ones:

Results:

                                                                                  improvement confidence      p.value
 child_process\\child-process-params.js params=1 methodName="exec" n=1000             -0.34 %            4.515010e-01
 child_process\\child-process-params.js params=1 methodName="execFile" n=1000          0.12 %            5.251958e-01
 child_process\\child-process-params.js params=1 methodName="execFileSync" n=1000      1.14 %            2.147775e-01
 child_process\\child-process-params.js params=1 methodName="execSync" n=1000          0.17 %            3.557482e-01
 child_process\\child-process-params.js params=1 methodName="spawn" n=1000             0.97 %        *** 4.384927e-05
 child_process\\child-process-params.js params=1 methodName="spawnSync" n=1000        -0.91 %            5.033024e-01
 child_process\\child-process-params.js params=2 methodName="exec" n=1000             -0.06 %            8.951647e-01
 child_process\\child-process-params.js params=2 methodName="execFile" n=1000         -0.12 %            5.468676e-01
 child_process\\child-process-params.js params=2 methodName="execFileSync" n=1000      0.14 %            8.581547e-01
 child_process\\child-process-params.js params=2 methodName="execSync" n=1000          0.01 %            9.721929e-01
 child_process\\child-process-params.js params=2 methodName="spawn" n=1000             1.05 %        *** 6.128593e-07
 child_process\\child-process-params.js params=2 methodName="spawnSync" n=1000        -0.99 %            3.905834e-01
 child_process\\child-process-params.js params=3 methodName="exec" n=1000              0.09 %            7.962740e-01
 child_process\\child-process-params.js params=3 methodName="execFile" n=1000          0.01 %            9.255018e-01
 child_process\\child-process-params.js params=3 methodName="execFileSync" n=1000     -1.30 %            1.967642e-01
 child_process\\child-process-params.js params=3 methodName="spawn" n=1000            -0.05 %            7.675626e-01
 child_process\\child-process-params.js params=3 methodName="spawnSync" n=1000        -0.81 %            5.329509e-01
 child_process\\child-process-params.js params=4 methodName="execFile" n=1000          0.05 %            6.862553e-01

[mscdex]

In that case maybe we don't need to backport? Unless it helps with the ease of backporting other child_process PRs?

[gibfahn]

All other things being equal, from a backporting perspective I'd rather minimize the v6.x...master delta where possible. The question is whether the risk is worth it.

Given that this has been in v7.x since March I'd say we're okay to backport, however I'd be interested to know how risky people think this PR is.

[gibfahn]

CI: https://ci.nodejs.org/job/node-test-commit/10668/

[gibfahn]

Single ARM failure looks like a flake, rerun: https://ci.nodejs.org/job/node-test-commit/10674/

[gibfahn]

@nodejs/performance anyone willing to review this (that it's okay for 6.x)?

[mcollina]

LGTM

[gibfahn]

Landed in 327f11f.

@vsemozhetbyt I see you moved var cmd = file; in the original PR, just wanted to confirm that this that this isn't necessary in v6.x.

[vsemozhetbyt]

@gibfahn Yes, this was due to https://bugs.chromium.org/p/v8/issues/detail?id=6010 which is not a case for Node.js v6.x