fs: fix deoptimizing use of `arguments`

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)

fs, benchmark

This is a twisted case that puzzles me for some time. I am not sure how it should proceed.

Prehistory / context

Thre are two deoptimization cases concerning arguments that affect fs module. They have similar conditions and affect almost the same fs functions.

I. 'Assignment to parameter in arguments object' in strict mode.

Previously this bailout was reported only for sloppy mode: see here and here. However, it seems there is some combination of circumstances for strict mode too.

There should be 3 conditions (which partly intersect with case II below):

1. arguments mention (any: arguments, arguments[valid_index], arguments.length).
2. Named parameter leaking in a lower scope.
3. Named parameter reassignment (it should be reassigned in the parent scope; reassignment in the lower scope does not cause deopt).

How to check (run with --trace_opt --trace_deopt and see the output):

function testFuncDeopt(namedArg) {
  const baz = arguments[0]; // condition 1
  namedArg = 'bar'; // condition 3
  function inner() { return namedArg; } // condition 2
}
for (let i = 0; i < 1e3; i++) testFuncDeopt('foo');

Versions impact:

v8 4.5.103.45 (Node.js 4.8.0) — affected (disabled optimization). v8 5.1.281.93 (Node.js 6.10.0) — partly affected (deopt, then new optimization). v8 5.5.372.41 (Node.js 7.7.1) — affected (disabled optimization). v8 5.6.326.55 (Node.js 8.0.0-nightly / master) — partly affected, partly replaced by the case II (see below). v8 5.8.202 (Node.js 8.0.0-pre vee-eight-lkgr) — partly affected, partly replaced by the case II (see below).

II. 'Bad value context for arguments value'.

This is a new deopt introduced by the v8 5.6 (see the upstream report; if I get this comment right, this will not be fixed till the new TurboFan pipeline hits the default use, i.e. till v8 5.9 (?)).

There should be 2 conditions (which partly intersect with case I):

1. Reading arguments[valid_index] (not arguments or arguments.length).
2. Named parameter leaking in a lower scope.

How to check (run with --trace_opt --trace_deopt and see the output):

function testFuncDeopt(namedArg) {
  const baz = arguments[0]; // condition 1
  function inner() { return namedArg; } // condition 2
}
for (let i = 0; i < 1e3; i++) testFuncDeopt('foo');

Versions impact:

v8 5.6.326.55 (Node.js 8.0.0-nightly / master) — affected. v8 5.8.202 (Node.js 8.0.0-pre vee-eight-lkgr) — affected.

Node.js libs impact

I've checked the libs for these two cases and have found out only 4 functions affected: 3 from fs (this PR) and 1 from _http_agent (will address in another PR later on).

fs lib impact

1. fs.readFile() has the conditions for the both cases I and II.
2. fs.writeFile() has the conditions for the both cases I and II.
3. Inner writeAll() has the conditions for the case I only.

I've tried to eliminate both the cases, so I had two variants of addressing: remove using of arguments or use intermediate variables for named parameters to transfer them into the lower scope. Both ways have the same benchmark results, so I've selected the first way (the second seems to be much more complicated and lumbering).

For the fs.readFile() and fs.writeFile() I've replaced arguments by conditional using of named parameters.

As to writeAll(), I've noticed that the check in the first line is redundant: writeAll() is called only by the fs.writeFile() method (which has its own check that throws if callback is not provided) and by the writeAll() itself with already checked callback. So we can remove the check and the intermediate parameter.

All tests are passed with the proposed changes (after this PR landed).

You can check deoptimization and fixes with these tests (run with --trace_opt --trace_deopt and see the output):

const readFile = require('fs').readFile;
const cb = () => {};
for (let i = 0; i < 1e3; i++) readFile(__filename, cb);

const writeFile = require('fs').writeFile;
const cb = () => {};
for (let i = 0; i < 1e3; i++) writeFile('testfile.txt', '1', cb);

Performance puzzle

After running a newly created benchmark, I've found out that the optimized fs.readFile() runs much slower than the unoptimized one on the master. fs.writeFile() has flaky results.

Some examples:

                                                                                 improvement confidence      p.value
 fs\\read-write-file-params.js withOptions="false" methodName="readFile" n=1000     -13.61 %        *** 3.628276e-06
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000      2.13 %            7.127870e-01
 fs\\read-write-file-params.js withOptions="true" methodName="readFile" n=1000       -9.00 %         ** 2.312356e-03
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000       2.38 %            5.706614e-01

                                                                                 improvement confidence     p.value
 fs\\read-write-file-params.js withOptions="false" methodName="readFile" n=1000      -9.47 %         ** 0.002279570
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000     10.82 %          * 0.030607596
 fs\\read-write-file-params.js withOptions="true" methodName="readFile" n=1000       -9.10 %         ** 0.004372376
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000      -3.62 %            0.391912744

                                                                                 improvement confidence     p.value
 fs\\read-write-file-params.js withOptions="false" methodName="readFile" n=1000      -6.93 %          * 0.034545206
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000     -0.62 %            0.902353347
 fs\\read-write-file-params.js withOptions="true" methodName="readFile" n=1000      -11.37 %         ** 0.001215041
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000      -0.38 %            0.945159471

                                                                                 improvement confidence      p.value
 fs\\read-write-file-params.js withOptions="false" methodName="readFile" n=1000      -7.67 %          * 0.0126620954
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000     -0.72 %            0.8910846613
 fs\\read-write-file-params.js withOptions="true" methodName="readFile" n=1000      -10.54 %        *** 0.0009612553
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000      -8.19 %            0.0663402207

Moreover, the fs.readFile() tests show a very strange new deoptimization: it happens after all the cycle runs, so it hardly has any performance impact but can be an evidence of some murky processes. See more in this issue.

I am not sure what to do with all this data. Maybe my benchmark is just wrong (I've gone over some alterations with no noticeable changes). So I just share it with the team by this PR to draw more thoughts. Sorry for any mess.